push initial setup #1
Conversation
WalkthroughAdds a multi-stage Dockerfile and Jenkins Kubernetes pipeline; updates container entry script with preflight checks; raises ClickHouse/metadata limits and defaults; and implements frontend changes for connection/time/auth wiring, per-source facet lazy-loading, configurable pagination pageSize, and multiple UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Container
participant Entry as /etc/local/entry.sh
participant API
participant App
Container->>Entry: start (ENTRYPOINT)
Entry->>Entry: print banner, run preflight checks (api dist, app .next)
alt preflight OK
Entry->>API: node -r ./packages/api/dist/tracing.js ./packages/api/dist/index.js
Entry->>App: cd ./packages/app && HOSTNAME='0.0.0.0' ... yarn start
else preflight fail
Entry-->>Container: exit 1
end
Note over Container: HEALTHCHECK: GET http://localhost:8000/health
sequenceDiagram
participant Dev
participant Jenkins
participant Pod
participant Dind
participant Builder
participant Registry
Dev->>Jenkins: push / PR
Jenkins->>Pod: create Pod (dind + builder)
Pod->>Dind: start Docker daemon (2375)
Builder->>Dind: connect via DOCKER_HOST
Jenkins->>Builder: armory build (GITHUB_TOKEN)
alt branch ∈ {main,staging,feature/*}
Jenkins->>Builder: armory push
Builder->>Registry: push image
end
sequenceDiagram
participant User
participant UI as DBSearchPageFilters
participant Meta as Metadata Service
User->>UI: open / interact (with sourceType/connection)
UI->>UI: getKeysForSourceType(sourceName,connectionName)
UI->>Meta: useGetKeyValues(initialKeys up to 50)
Meta-->>UI: initial facet values
User->>UI: click "load more" for key
UI->>Meta: fetch additional values for key (limit up to 1000)
Meta-->>UI: extra values
UI->>UI: merge into extraFacets -> update shown facets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
📜 Recent review detailsConfiguration used: Repository: ShareChat/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
489-521: Consider pagination for large datasetsThe
loadMoreFilterValuesForKeyfunction fetches a fixed limit of 200 values without offset/pagination support. For keys with more than 200 unique values, users won't be able to access all values.Consider implementing pagination by tracking the offset for each key:
const [extraFacets, setExtraFacets] = useState<Record<string, string[]>>({}); +const [keyOffsets, setKeyOffsets] = useState<Record<string, number>>({}); const loadMoreFilterValuesForKey = useCallback( async (key: string) => { setLoadMoreLoadingKeys(prev => new Set(prev).add(key)); try { const metadata = getMetadata(); + const currentOffset = keyOffsets[key] || keyLimit; const newKeyVals = await metadata.getKeyValues({ chartConfig: { ...chartConfig, dateRange, }, keys: [key], limit: 200, + offset: currentOffset, disableRowLimit: true, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(1 hunks)Jenkinsfile(1 hunks)docker/hyperdx/entry.prod.sh(1 hunks)packages/app/src/components/DBSearchPageFilters.tsx(3 hunks)
🔇 Additional comments (7)
Jenkinsfile (2)
63-86: Pipeline stages look goodThe build and push stages are appropriately configured with branch filtering for deployments.
55-61: Confirm Docker build usage and secret management
I didn’t find anydocker buildordocker.buildinvocations in yourJenkinsfile, nor anARG GITHUB_TOKENin yourDockerfile. Please verify that you’re not accidentally passingGITHUB_TOKENinto your image layers. Consider using BuildKit secrets or multi-stage builds to keep credentials out of final artifacts.
- Search all
shor pipeline steps for any--build-arg buildarg_GITHUB_TOKEN=…usage.- Ensure your
Dockerfiledoesn’t declare or consumeARG GITHUB_TOKEN.- If you must pass secrets at build time, switch to BuildKit’s
--secretmechanism or bind credentials via Jenkins plugins.docker/hyperdx/entry.prod.sh (2)
15-24: Good addition of preflight checksThe preflight checks ensure required build artifacts exist before attempting to start services, providing clear error messages for debugging.
30-32: Service startup commands properly updatedThe commands correctly reference the built distribution files and use appropriate startup methods for production deployment.
Dockerfile (3)
1-8: Well-structured multi-stage DockerfileGood use of multi-stage builds with Alpine base image for smaller footprint. The NODE_VERSION ARG provides flexibility.
55-60: Good security practice with non-root userRunning the container as a non-root user is a security best practice. The ownership is properly set with
--chown.
20-22: Confirm skip-build implicationsI ran a search across all
package.jsonfiles and found nopostinstallscripts defined, soyarn install --mode=skip-buildwon’t skip any project-level lifecycle hooks. However, note that--mode=skip-buildalso suppresses dependency install scripts and native module builds. Please verify:
- None of your dependencies rely on
postinstall,prepare, or other install-time scripts to function correctly.- Any required native modules or build steps are handled elsewhere (e.g., prebuilt binaries or CI jobs).
| ARG CODE_VERSION=2.1.1 | ||
|
|
||
| ENV CODE_VERSION=2.1.1 |
There was a problem hiding this comment.
Fix CODE_VERSION environment variable
The ENV CODE_VERSION=2.1.1 hardcodes the value and overrides the ARG, making the build argument ineffective.
ARG CODE_VERSION=2.1.1
-ENV CODE_VERSION=2.1.1
+ENV CODE_VERSION=${CODE_VERSION}🤖 Prompt for AI Agents
In Dockerfile around lines 47 to 49, the ENV line hardcodes CODE_VERSION and
overrides the ARG; replace the hardcoded ENV with one that references the build
ARG (e.g. ENV CODE_VERSION=${CODE_VERSION}) or remove the ENV entirely if you
only need the ARG during build time, ensuring the ARG value is propagated
correctly to the environment when required.
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD curl -f http://localhost:8000/health || exit 1 |
There was a problem hiding this comment.
Ensure curl is available for health check
The HEALTHCHECK uses curl but it's not explicitly installed in the production stage. Alpine images may not include curl by default.
Add curl installation in the production stage:
FROM node:${NODE_VERSION}-alpine AS production
ARG CODE_VERSION=2.1.1
+# Install curl for healthcheck
+RUN apk add --no-cache curl
+
ENV CODE_VERSION=${CODE_VERSION}
ENV NODE_ENV production📝 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.
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD curl -f http://localhost:8000/health || exit 1 | |
| FROM node:${NODE_VERSION}-alpine AS production | |
| ARG CODE_VERSION=2.1.1 | |
| # Install curl for healthcheck | |
| RUN apk add --no-cache curl | |
| ENV CODE_VERSION=${CODE_VERSION} | |
| ENV NODE_ENV production |
🤖 Prompt for AI Agents
In Dockerfile around lines 91-92, the HEALTHCHECK uses curl but the production
stage doesn't install it; add a package install for curl in that stage before
the HEALTHCHECK. If the base image is Alpine add a line like "RUN apk add
--no-cache curl", if it's Debian/Ubuntu add "RUN apt-get update && apt-get
install -y curl && rm -rf /var/lib/apt/lists/*", placing it in the production
stage so curl is available for the HEALTHCHECK command.
| containers: | ||
| - name: dind | ||
| image: sc-mum-armory.platform.internal/devops/dind:v2 | ||
| securityContext: | ||
| privileged: true | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: tcp://localhost:2375 | ||
| - name: DOCKER_TLS_CERTDIR | ||
| value: "" | ||
| volumeMounts: | ||
| - name: dind-storage | ||
| mountPath: /var/lib/docker | ||
| readinessProbe: | ||
| tcpSocket: | ||
| port: 2375 | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 10 | ||
| livenessProbe: | ||
| tcpSocket: | ||
| port: 2375 | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 20 | ||
| - name: builder | ||
| image: sc-mum-armory.platform.internal/devops/builder-image-armory | ||
| command: | ||
| - sleep | ||
| - infinity | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: tcp://localhost:2375 | ||
| - name: DOCKER_BUILDKIT | ||
| value: "0" | ||
| volumeMounts: | ||
| - name: jenkins-sa | ||
| mountPath: /root/.gcp/ | ||
| volumes: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource limits for containers
The Pod specification lacks resource limits and requests for both containers, which could lead to resource contention and pod instability.
Add resource limits to prevent unbounded resource consumption:
spec:
containers:
- name: dind
image: sc-mum-armory.platform.internal/devops/dind:v2
+ resources:
+ requests:
+ memory: "512Mi"
+ cpu: "500m"
+ limits:
+ memory: "2Gi"
+ cpu: "2000m"
securityContext:
privileged: true - name: builder
image: sc-mum-armory.platform.internal/devops/builder-image-armory
+ resources:
+ requests:
+ memory: "1Gi"
+ cpu: "1000m"
+ limits:
+ memory: "4Gi"
+ cpu: "4000m"
command:
- sleep📝 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.
| containers: | |
| - name: dind | |
| image: sc-mum-armory.platform.internal/devops/dind:v2 | |
| securityContext: | |
| privileged: true | |
| env: | |
| - name: DOCKER_HOST | |
| value: tcp://localhost:2375 | |
| - name: DOCKER_TLS_CERTDIR | |
| value: "" | |
| volumeMounts: | |
| - name: dind-storage | |
| mountPath: /var/lib/docker | |
| readinessProbe: | |
| tcpSocket: | |
| port: 2375 | |
| initialDelaySeconds: 30 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| tcpSocket: | |
| port: 2375 | |
| initialDelaySeconds: 30 | |
| periodSeconds: 20 | |
| - name: builder | |
| image: sc-mum-armory.platform.internal/devops/builder-image-armory | |
| command: | |
| - sleep | |
| - infinity | |
| env: | |
| - name: DOCKER_HOST | |
| value: tcp://localhost:2375 | |
| - name: DOCKER_BUILDKIT | |
| value: "0" | |
| volumeMounts: | |
| - name: jenkins-sa | |
| mountPath: /root/.gcp/ | |
| volumes: | |
| containers: | |
| - name: dind | |
| image: sc-mum-armory.platform.internal/devops/dind:v2 | |
| resources: | |
| requests: | |
| memory: "512Mi" | |
| cpu: "500m" | |
| limits: | |
| memory: "2Gi" | |
| cpu: "2000m" | |
| securityContext: | |
| privileged: true | |
| env: | |
| - name: DOCKER_HOST | |
| value: tcp://localhost:2375 | |
| - name: DOCKER_TLS_CERTDIR | |
| value: "" | |
| volumeMounts: | |
| - name: dind-storage | |
| mountPath: /var/lib/docker | |
| readinessProbe: | |
| tcpSocket: | |
| port: 2375 | |
| initialDelaySeconds: 30 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| tcpSocket: | |
| port: 2375 | |
| initialDelaySeconds: 30 | |
| periodSeconds: 20 | |
| - name: builder | |
| image: sc-mum-armory.platform.internal/devops/builder-image-armory | |
| resources: | |
| requests: | |
| memory: "1Gi" | |
| cpu: "1000m" | |
| limits: | |
| memory: "4Gi" | |
| cpu: "4000m" | |
| command: | |
| - sleep | |
| - infinity | |
| env: | |
| - name: DOCKER_HOST | |
| value: tcp://localhost:2375 | |
| - name: DOCKER_BUILDKIT | |
| value: "0" | |
| volumeMounts: | |
| - name: jenkins-sa | |
| mountPath: /root/.gcp/ | |
| volumes: |
🤖 Prompt for AI Agents
In Jenkinsfile around lines 9 to 45, both the dind and builder container specs
are missing resource requests/limits; add a resources block under each container
with requests and limits for cpu and memory (and optionally ephemeral-storage)
to prevent unbounded consumption; set sensible values (e.g. requests smaller
than limits) appropriate for CI workloads and your cluster, validate units (m
for millicpu, Mi/Gi for memory), and adjust values after load testing or per
team policy.
| - name: dind | ||
| image: sc-mum-armory.platform.internal/devops/dind:v2 | ||
| securityContext: | ||
| privileged: true | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: tcp://localhost:2375 | ||
| - name: DOCKER_TLS_CERTDIR | ||
| value: "" |
There was a problem hiding this comment.
Security concern: Privileged container without TLS
The dind container runs with privileged: true and DOCKER_TLS_CERTDIR: "" which disables TLS encryption for the Docker daemon. This exposes the Docker API without encryption.
Consider enabling TLS for the Docker daemon or document why this security trade-off is acceptable in your environment.
🤖 Prompt for AI Agents
In the Jenkinsfile, around lines 10-18, the dind container is running in
privileged mode without TLS encryption enabled for the Docker daemon. This poses
a security concern as it exposes the Docker API without encryption. To address
this issue, either enable TLS encryption for the Docker daemon or document the
rationale for accepting this security trade-off in your environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
420-433: Add TypeScript type for thesourceTypeprop.The
sourceTypeprop is added but its type definition is missing from the component's props interface.Add the type definition:
}: { analysisMode: 'results' | 'delta' | 'pattern'; setAnalysisMode: (mode: 'results' | 'delta' | 'pattern') => void; isLive: boolean; chartConfig: ChartConfigWithDateRange; sourceId?: string; + sourceType?: string; showDelta: boolean; denoiseResults: boolean; setDenoiseResults: (denoiseResults: boolean) => void; } & FilterStateHook) => {
♻️ Duplicate comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
459-459: Remove debug console statement.Debug console.error statement should be removed before merging.
- console.error('nishant is here === ', sourceType);
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
526-526: Remove unusedsourceTypefrom dependency array.The
sourceTypeis not used within theloadMoreFilterValuesForKeycallback but is included in its dependencies, which could trigger unnecessary re-renders.- [chartConfig, setExtraFacets, dateRange, sourceType], + [chartConfig, setExtraFacets, dateRange],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(6 hunks)
🔇 Additional comments (4)
packages/app/src/components/DBSearchPageFilters.tsx (4)
32-62: LGTM! Well-structured source type overrides.The service map configuration and helper function provide a clean way to customize facet keys per source type while maintaining default behavior for unmapped types.
205-209: Good implementation of auto-loading behavior.The effect correctly triggers lazy loading for filter values when the component mounts, ensuring data is available without user interaction.
487-488: Correct usage of source type overrides for key fetching.The implementation properly applies the source-specific keys when fetching facet values.
531-579: Well-implemented facet merging and ordering logic.The code properly handles merging of lazy-loaded facets, ensures all required keys are present, and maintains correct ordering with pinned fields prioritized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
456-466: Remove leftover debug log and clarify fallback behavior.The console.error is a stray debug statement and should be removed. Also, relying on an empty array to trigger “default behavior” depends on hook semantics and may fetch nothing.
Apply this cleanup:
const keysToFetch = useMemo(() => { // Override keys for specific source types if (sourceType && sourceType in serviceMapOverride) { return getKeysForSourceType(sourceType); } - console.error('nishant is here === ', sourceType); // For other source types, return empty array to use default behavior // This allows the system to fetch all available keys from metadata return []; }, [sourceType]);Verify hook behavior when keys is []:
#!/bin/bash # Locate and inspect useGetKeyValues to confirm how it treats keys: [] rg -n "function useGetKeyValues|const useGetKeyValues|export function useGetKeyValues|export const useGetKeyValues" -A 50 rg -n "keys:" -A 5 -B 5
🧹 Nitpick comments (5)
packages/app/src/components/DBSearchPageFilters.tsx (5)
55-61: getKeysForSourceType should be strongly typed to SourceKind to avoid string drift.Using string comparisons here is error-prone. Prefer SourceKind.
// outside current diff range: import { SourceKind } from '@hyperdx/common-utils/dist/types'; const getKeysForSourceType = (sourceType?: SourceKind) => sourceType ? serviceMapOverride[sourceType] ?? [] : [];
487-490: Use the memoized keysToFetch to avoid duplicate logic and ensure consistency.Currently you compute keys twice and rely on comments about default behavior. Use the same logic everywhere.
const { data: facets, isLoading: isFacetsLoading, isFetching: isFacetsFetching, } = useGetKeyValues({ chartConfigs: { ...chartConfig, dateRange }, limit: keyLimit, - keys: getKeysForSourceType(sourceType), + keys: keysToFetch, });
494-529: Trim loadMoreFilterValuesForKey dependencies; sourceType isn’t used.sourceType in deps causes unnecessary re-creations. Remove it unless you plan to vary behavior by sourceType.
- [chartConfig, setExtraFacets, dateRange, sourceType], + [chartConfig, setExtraFacets, dateRange],
206-211: Auto “load more” on mount may fan-out requests for every facet.This effect triggers network fetches for all groups as soon as they render. Consider gating behind user intent (e.g., only when expanded or when group is visible) to avoid N simultaneous requests on first paint.
Example:
- useEffect(() => { - if (onLoadMore && !hasLoadedMore) { - onLoadMore(name); - } - }, [onLoadMore, hasLoadedMore, name]); + useEffect(() => { + if (onLoadMore && isExpanded && !hasLoadedMore) { + onLoadMore(name); + } + }, [onLoadMore, hasLoadedMore, name, isExpanded]);If intentional, please confirm backend can handle this fan-out without rate limiting.
445-447: Remove unused variable numRows.numRows is computed but unused; likely leftover.
- const { data: countData } = useExplainQuery(chartConfig); - const numRows: number = countData?.[0]?.rows ?? 0; + const { data: countData } = useExplainQuery(chartConfig);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/app/src/DBSearchPage.tsx(1 hunks)packages/app/src/components/DBSearchPageFilters.tsx(7 hunks)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
466-512: Drop console logs (no-console), simplify keysToFetch, and add missing dependency.
- Remove debug console statements flagged by ESLint.
- Prefer early return using getKeysForSourceType; fallback to data-derived keys only when no override exists.
- Include isFieldPinned in deps so changes to pinning recompute keys reliably.
- const keysToFetch = useMemo(() => { - console.log('🔍 keysToFetch - sourceType:', sourceType, 'available overrides:', Object.keys(serviceMapOverride)); - - // First check if we have a source type override - if (sourceType && sourceType in serviceMapOverride) { - const overrideKeys = serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - console.log('✅ Using source type override for', sourceType, ':', overrideKeys); - return overrideKeys; - } - - console.log('⚠️ No source type override found for', sourceType, '- falling back to data-based logic'); - - // If no source type override, fall back to data-based logic + const keysToFetch = useMemo(() => { + // Prefer source-type override; otherwise fall back to data-derived keys + const override = getKeysForSourceType(sourceType); + if (override.length > 0) return override; + + // Fallback: derive from metadata if (!data) { return []; } @@ - return strings; - }, [sourceType, data, filterState, showMoreFields]); + return strings; + }, [sourceType, data, filterState, showMoreFields, isFieldPinned]);
🧹 Nitpick comments (4)
packages/app/src/components/DBSearchPageFilters.tsx (4)
211-216: Auto “load more” on mount can cause bursty background fetches. Confirm intent.This effect will prefetch “more values” for every facet as soon as it renders. If facet count is large, this can generate high request volume.
Consider gating:
- Only autoload for pinned fields or the first N visible facets.
- Trigger when expanded/visible (IntersectionObserver), or debounce on first hover/click.
461-465: Remove commented dead code.The old keysToFetch block is commented out and should be removed for clarity.
- // const keysToFetch = useMemo(() => { - // // Get keys for the source type - will return specific keys if available, or default keys otherwise - // return getKeysForSourceType(sourceType); - // }, [sourceType]);
426-437: Type sourceType to SourceKind for compile-time safety.This prevents key drift and ensures overrides apply correctly. You’re already importing from the same module.
- sourceType?: string; + sourceType?: SourceKind;Add import (outside current diff range):
import { ChartConfigWithDateRange, SourceKind } from '@hyperdx/common-utils/dist/types';
574-575: Tighten useCallback deps (avoid unnecessary identity churn).sourceType and setExtraFacets aren’t used inside the callback; removing them reduces re-renders in children relying on onLoadMore identity.
- [chartConfig, setExtraFacets, dateRange, sourceType], + [chartConfig, dateRange],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(8 hunks)
🧰 Additional context used
🪛 ESLint
packages/app/src/components/DBSearchPageFilters.tsx
[error] 32-32: Delete ··
(prettier/prettier)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 35-35: Delete ··
(prettier/prettier)
[error] 36-36: Delete ··
(prettier/prettier)
[error] 37-37: Replace ······ with ····
(prettier/prettier)
[error] 38-38: Delete ··
(prettier/prettier)
[error] 39-39: Delete ··
(prettier/prettier)
[error] 40-40: Replace ···· with ··
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
[error] 42-42: Delete ··
(prettier/prettier)
[error] 43-43: Delete ··
(prettier/prettier)
[error] 44-44: Replace ······ with ····
(prettier/prettier)
[error] 45-45: Delete ··
(prettier/prettier)
[error] 46-46: Delete ··
(prettier/prettier)
[error] 47-51: Replace ····session:·[⏎······'ServiceName',⏎······'StatusCode',⏎······'SpanKind',⏎···· with ··session:·['ServiceName',·'StatusCode',·'SpanKind'
(prettier/prettier)
[error] 52-56: Replace ····metric:·[⏎······'ServiceName',⏎······'MetricName',⏎······'Unit',⏎···· with ··metric:·['ServiceName',·'MetricName',·'Unit'
(prettier/prettier)
[error] 57-57: Delete ··
(prettier/prettier)
[error] 466-466: Delete ·
(prettier/prettier)
[error] 467-467: Unexpected console statement.
(no-console)
[error] 467-467: Replace '🔍·keysToFetch·-·sourceType:',·sourceType,·'available·overrides:',·Object.keys(serviceMapOverride) with ⏎······'🔍·keysToFetch·-·sourceType:',⏎······sourceType,⏎······'available·overrides:',⏎······Object.keys(serviceMapOverride),⏎····
(prettier/prettier)
[error] 471-471: Insert ⏎·······
(prettier/prettier)
[error] 472-472: Unexpected console statement.
(no-console)
[error] 472-472: Replace '✅·Using·source·type·override·for',·sourceType,·':',·overrideKeys with ⏎········'✅·Using·source·type·override·for',⏎········sourceType,⏎········':',⏎········overrideKeys,⏎······
(prettier/prettier)
[error] 476-476: Unexpected console statement.
(no-console)
[error] 476-476: Replace '⚠️·No·source·type·override·found·for',·sourceType,·'-·falling·back·to·data-based·logic' with ⏎······'⚠️·No·source·type·override·found·for',⏎······sourceType,⏎······'-·falling·back·to·data-based·logic',⏎····
(prettier/prettier)
🔇 Additional comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
59-66: Normalize sourceType to ensure override lookup succeeds.Lowercasing sourceType before lookup fixes the mismatch and keeps the override logic centralized.
-const getKeysForSourceType = (sourceType?: string) => { - if (sourceType && sourceType in serviceMapOverride) { - return serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - } - // For other source types, return empty array to use default behavior - return []; -}; +const getKeysForSourceType = (sourceType?: string) => { + const key = sourceType?.toLowerCase().trim(); + if (key && key in serviceMapOverride) { + return serviceMapOverride[key as keyof typeof serviceMapOverride]; + } + // For other source types, return empty array to use default behavior + return []; +};Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
454-513: Remove all debug console statements and fix typoMultiple debug console.error statements need to be removed before merging. Also, there's a typo: "erroric" should be "logic" on lines 469 and 471.
Remove all console statements:
const keysToFetch = useMemo(() => { - console.error('🔍 keysToFetch function called with:'); - console.error(' - sourceType:', sourceType); - console.error(' - data available:', !!data); - console.error(' - data length:', data?.length || 0); - console.error(' - serviceMapOverride keys:', Object.keys(serviceMapOverride)); - // First check if we have a source type override if (sourceType && sourceType in serviceMapOverride) { const overrideKeys = serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - console.error('✅ Using source type override for', sourceType, ':', overrideKeys); return overrideKeys; } - console.error('⚠️ No source type override found for', sourceType, '- falling back to data-based erroric'); - - // If no source type override, fall back to data-based erroric + // If no source type override, fall back to data-based logic if (!data) { - console.error('❌ No data available, returning empty array'); return []; } - console.error('📊 Processing data for keysToFetch:'); - console.error(' - Raw data sample:', data.slice(0, 3)); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - filterState keys:', Object.keys(filterState)); const strings = data // ... rest of the logic - console.error('🎯 Final keysToFetch result:', strings); - console.error(' - Total keys found:', strings.length); - console.error(' - Keys:', strings); - return strings;
♻️ Duplicate comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
534-538: Use keysToFetch for consistencyThe
useGetKeyValueshook should use the computedkeysToFetchvariable which includes fallback logic, rather than callinggetKeysForSourceTypedirectly which may return an empty array.const { data: facets, isLoading: isFacetsLoading, isFetching: isFacetsFetching, } = useGetKeyValues({ chartConfigs: { ...chartConfig, dateRange }, limit: keyLimit, - keys: getKeysForSourceType(sourceType), + keys: keysToFetch, });
32-58: Critical: Source type overrides won't apply due to case mismatchThe
serviceMapOverrideuses lowercase keys ('log', 'trace'), butDBSearchPagepasses capitalized SourceKind values ('Log', 'Trace'). This case-sensitive mismatch means overrides will never be applied.Apply this fix to normalize the sourceType before lookup:
const getKeysForSourceType = (sourceType?: string) => { - if (sourceType && sourceType in serviceMapOverride) { - return serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; + if (sourceType) { + const normalizedType = sourceType.toLowerCase(); + if (normalizedType in serviceMapOverride) { + return serviceMapOverride[normalizedType as keyof typeof serviceMapOverride]; + } } // For other source types, return empty array to use default behavior return []; };Alternatively, update the keys to match SourceKind values:
const serviceMapOverride = { - log: [ + Log: [ 'SeverityText', // ... ], - trace: [ + Trace: [ 'ServiceName', // ... ], // ... };
🧹 Nitpick comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
545-577: Consider using proper logger instead of console.errorThe error handling uses
console.errorwhich should be replaced with a proper logging mechanism.} catch (error) { - console.error('failed to fetch more keys', error); + // Use your app's logger here + // logger.error('Failed to fetch more filter values', { key, error }); } finally {
459-465: Fix formatting issues flagged by ESLintRun Prettier to fix the formatting issues in the console.error statements (though these should be removed anyway as per the previous comment).
npx prettier --write packages/app/src/components/DBSearchPageFilters.tsx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)
🪛 ESLint
packages/app/src/components/DBSearchPageFilters.tsx
[error] 459-459: Replace '··-·serviceMapOverride·keys:',·Object.keys(serviceMapOverride) with ⏎······'··-·serviceMapOverride·keys:',⏎······Object.keys(serviceMapOverride),⏎····
(prettier/prettier)
[error] 465-465: Replace '✅·Using·source·type·override·for',·sourceType,·':',·overrideKeys with ⏎········'✅·Using·source·type·override·for',⏎········sourceType,⏎········':',⏎········overrideKeys,⏎······
(prettier/prettier)
[error] 469-469: Replace '⚠️·No·source·type·override·found·for',·sourceType,·'-·falling·back·to·data-based·erroric' with ⏎······'⚠️·No·source·type·override·found·for',⏎······sourceType,⏎······'-·falling·back·to·data-based·erroric',⏎····
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
455-525: Remove extensive debug logging and fix ESLint error.The implementation logic for keysToFetch is correct - it prioritizes source type overrides and falls back to data-driven logic. However, there are several issues that need to be addressed:
- Extensive console.error statements should be removed before production
- ESLint formatting error on line 491
- Potential performance impact from excessive logging
Apply this diff to remove debug logging:
const keysToFetch = useMemo(() => { - console.error('🔍 keysToFetch function called with:'); - console.error(' - sourceType:', sourceType); - console.error(' - data available:', !!data); - console.error(' - data length:', data?.length || 0); - console.error( - ' - serviceMapOverride keys:', - Object.keys(serviceMapOverride), - ); - // First check if we have a source type override if (sourceType && sourceType in serviceMapOverride) { const overrideKeys = serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - console.error( - '✅ Using source type override for', - sourceType, - ':', - overrideKeys, - ); return overrideKeys; } - console.error( - '⚠️ No source type override found for', - sourceType, - '- falling back to data-based logic', - ); - // If no source type override, fall back to data-based logic if (!data) { - console.error('❌ No data available, returning empty array'); return []; } - console.error('📊 Processing data for keysToFetch:'); - console.error(' - Raw data sample:', data.slice(0, 3)); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - filterState keys:', Object.keys(filterState)); const strings = data .sort((a, b) => { // First show low cardinality fields const isLowCardinality = (type: string) => type.includes('LowCardinality'); return isLowCardinality(a.type) && !isLowCardinality(b.type) ? -1 : 1; }) .filter( field => field.jsType && ['string'].includes(field.jsType), // todo: add number type with sliders :D ) .map(({ path, type }) => { return { type, path: mergePath(path) }; }) .filter( field => showMoreFields || field.type.includes('LowCardinality') || // query only low cardinality fields by default Object.keys(filterState).includes(field.path) || // keep selected fields isFieldPinned(field.path), // keep pinned fields ) .map(({ path }) => path) .filter( path => !['body', 'timestamp', '_hdx_body'].includes(path.toLowerCase()), ); - console.error('🎯 Final keysToFetch result:', strings); - console.error(' - Total keys found:', strings.length); - console.error(' - Keys:', strings); - return strings; }, [sourceType, data, filterState, showMoreFields]);
549-549: Inconsistent key fetching approach.This line uses
getKeysForSourceType(sourceType)directly, which can return an empty array and diverge from the computedkeysToFetchthat includes fallback logic. This inconsistency was flagged in previous reviews.- keys: getKeysForSourceType(sourceType), + keys: keysToFetch,
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
449-452: Remove commented code before merging.This commented-out code should be removed to keep the codebase clean.
- // const keysToFetch = useMemo(() => { - // // Get keys for the source type - will return specific keys if available, or default keys otherwise - // return getKeysForSourceType(sourceType); - // }, [sourceType]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)
🪛 ESLint
packages/app/src/components/DBSearchPageFilters.tsx
[error] 491-491: Replace '··-·filterState·keys:',·Object.keys(DBSearchPageFiltersComponent) with ⏎······'··-·filterState·keys:',⏎······Object.keys(DBSearchPageFiltersComponent),⏎····
(prettier/prettier)
🔇 Additional comments (6)
packages/app/src/components/DBSearchPageFilters.tsx (6)
94-96: LGTM! Well-documented empty function.The emptyFn utility with clear documentation explaining why it's intentionally empty is good practice for avoiding double-firing of checkbox events.
205-209: Auto-trigger load more functionality looks correct.The useEffect automatically triggers onLoadMore when the function exists and the data hasn't been loaded yet, which provides good UX for lazy loading.
420-420: sourceType prop integration is correct.The component now properly accepts sourceType as an optional string prop, addressing the previous issue where sourceType was undefined.
Also applies to: 430-430
552-589: Lazy loading implementation is well-structured.The extraFacets state management and loadMoreFilterValuesForKey function provide a solid implementation for lazy loading additional facet values. The error handling and loading state management are appropriate.
591-641: Enhanced shownFacets logic handles multiple data sources correctly.The implementation properly merges facets from multiple sources (API response, extra facets, filter state, and keysToFetch) and handles the reordering to prioritize pinned fields. The logic ensures all expected keys are represented even with empty values.
32-58: Formatting issues resolvedI ran
npx prettier --write packages/app/src/components/DBSearchPageFilters.tsxand the file was formatted successfully with no remaining ESLint or formatting errors.Note: the CLI emitted warnings about deprecated Prettier options (
jsxBracketSameLineand{ fluid: false }) in your Prettier configuration. You may want to update your config to remove or replace these outdated settings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/app/src/components/DBSearchPageFilters.tsx (4)
32-58: Potential key mismatch between override map and sourceType values.Based on past review comments, there were issues with sourceType values not matching the override keys. The current implementation uses lowercase keys (
log,trace), but ifsourceTypecomes fromSourceKindenum values (typically capitalized likeLog,Trace), the lookups will still fail.Verify that the
sourceTypevalues passed to this component actually match these lowercase keys, or update the keys to match the actualSourceKindvalues.
488-562: Remove debug logging and approve the core logic.The keysToFetch logic correctly prioritizes source-type overrides before falling back to data-based key selection. However, extensive debug logging from lines 489-560 must be removed.
Remove all
console.errorstatements within this useMemo block while keeping the core logic intact.
586-587: Use keysToFetch for consistency instead of calling getKeysForSourceType directly.As noted in previous reviews, using
getKeysForSourceType(sourceType)directly can return an empty array and diverge from the computedkeysToFetchwhich includes fallback logic.- keys: getKeysForSourceType(sourceType), + keys: keysToFetch,
435-482: Remove all debug console statements.Extensive debug logging remains throughout the component despite past review comments requesting their removal. This debug output should not be in production code.
Remove all
console.errorstatements from lines 435-482 and throughout the file (also present at lines ~489-560).
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
594-626: Remove unused dependency from useCallback.The
loadMoreFilterValuesForKeyfunction implementation is solid with proper error handling and state management. However,sourceTypeis included in the dependency array but not used within the function.- [chartConfig, setExtraFacets, dateRange, sourceType], + [chartConfig, setExtraFacets, dateRange],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (3)
packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (1)
useAllFields(82-126)
🪛 ESLint
packages/app/src/components/DBSearchPageFilters.tsx
[error] 525-528: Replace ⏎······'··-·filterState·keys:',⏎······Object.keys(filterState),⏎···· with '··-·filterState·keys:',·Object.keys(filterState)
(prettier/prettier)
🔇 Additional comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
205-209: Good addition for auto-triggering lazy load on mount.The useEffect correctly auto-triggers
onLoadMorewhen the component mounts if data hasn't been loaded yet. This enhances the user experience by proactively loading facet values.
628-678: Excellent integration of lazy-loaded facet values.The
shownFacetslogic properly merges extra facets with regular facets, ensures allkeysToFetchkeys are represented, and maintains the existing sorting behavior with pinned fields prioritized.The implementation handles all edge cases correctly and provides a seamless user experience for the lazy-loading functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
488-559: Remove debug console statements from keysToFetch computationThe keysToFetch logic is correct, but contains extensive debug logging that should be removed.
const keysToFetch = useMemo(() => { - console.error('🔍 keysToFetch function called with:'); - console.error(' - sourceType:', sourceType); - console.error(' - data available:', !!data); - console.error(' - data length:', data?.length || 0); - console.error( - ' - serviceMapOverride keys:', - Object.keys(serviceMapOverride), - ); // First check if we have a source type override if (sourceType && sourceType in serviceMapOverride) { const overrideKeys = serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - console.error( - '✅ Using source type override for', - sourceType, - ':', - overrideKeys, - ); return overrideKeys; } - console.error( - '⚠️ No source type override found for', - sourceType, - '- falling back to data-based logic', - ); // If no source type override, fall back to data-based logic if (!data) { - console.error('❌ No data available, returning empty array'); return []; } - console.error('📊 Processing data for keysToFetch:'); - console.error(' - Raw data sample:', data.slice(0, 3)); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - filterState keys:', Object.keys(filterState)); // ... rest of the logic - console.error('🎯 Final keysToFetch result:', strings); - console.error(' - Total keys found:', strings.length); - console.error(' - Keys:', strings); return strings; }, [sourceType, data, filterState, showMoreFields]);
♻️ Duplicate comments (3)
packages/app/src/components/DBSearchPageFilters.tsx (3)
32-58: Fix case mismatch in serviceMapOverride keysThe override map uses lowercase keys (
"log","trace"), butDBSearchPagepassesSourceKindvalues which are capitalized (e.g.,"Log","Trace"). This causes the override lookup to fail.Apply this fix to align keys with SourceKind values:
const serviceMapOverride = { - log: [ + Log: [ 'SeverityText', 'ServiceName', "ResourceAttributes['k8s.cluster.name']", "ResourceAttributes['k8s.namespace.name']", ], - trace: [ + Trace: [ 'ServiceName', 'StatusCode', "ResourceAttributes['k8s.node.name']", "ResourceAttributes['k8s.owner.name']", 'SpanKind', ], session: ['ServiceName', 'StatusCode', 'SpanKind'], metric: ['ServiceName', 'MetricName', 'Unit'], };Also fix the ESLint formatting issues by running:
npx prettier --write packages/app/src/components/DBSearchPageFilters.tsx
583-584: Use keysToFetch instead of calling getKeysForSourceType directlyThe
useGetKeyValueshook should use the computedkeysToFetchvalue which includes fallback logic, rather than callinggetKeysForSourceTypedirectly.const { data: facets, isLoading: isFacetsLoading, isFetching: isFacetsFetching, } = useGetKeyValues({ chartConfigs: { ...chartConfig, dateRange }, limit: keyLimit, - keys: getKeysForSourceType(sourceType), + keys: keysToFetch, });
436-481: Remove all debug console statements before mergingThese extensive debug statements should not be in production code. They use
console.errorinappropriately and will clutter the console.Remove all console statements:
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = usePinnedFilters(sourceId ?? null); - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); const { width, startResize } = useResizable(16, 'left'); - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); // ... (remove other console statements)
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
622-622: Remove unnecessary sourceType from dependency arrayThe
loadMoreFilterValuesForKeycallback doesn't usesourceType, so it shouldn't be in the dependency array.- [chartConfig, setExtraFacets, dateRange, sourceType], + [chartConfig, setExtraFacets, dateRange],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (3)
packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (1)
useAllFields(82-126)
|
|
||
| return strings; | ||
| }, [data, filterState, showMoreFields]); | ||
| }, [sourceType, data, filterState, showMoreFields]); |
There was a problem hiding this comment.
Add missing isFieldPinned to dependency array
The keysToFetch useMemo uses isFieldPinned but doesn't include it in the dependency array.
- }, [sourceType, data, filterState, showMoreFields]);
+ }, [sourceType, data, filterState, showMoreFields, isFieldPinned]);📝 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.
| }, [sourceType, data, filterState, showMoreFields]); | |
| }, [sourceType, data, filterState, showMoreFields, isFieldPinned]); |
🤖 Prompt for AI Agents
In packages/app/src/components/DBSearchPageFilters.tsx around line 559, the
useMemo that computes keysToFetch references isFieldPinned but doesn't list it
in the dependency array; update the dependency array to include isFieldPinned
(or if isFieldPinned is a function prop, ensure it's stable by wrapping it with
useCallback and then include it) so the memo recalculates when isFieldPinned
changes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/common-utils/src/metadata.ts (1)
459-461: Bug: cache key omits thelimitparameter, causing incorrect cache hitsRequests with different
limitvalues will share the same cache entry, returning stale/incorrect results.- return this.cache.getOrFetch( - `${chartConfig.from.databaseName}.${chartConfig.from.tableName}.${keys.join(',')}.${chartConfig.dateRange.toString()}.${disableRowLimit}.values`, + return this.cache.getOrFetch( + `${chartConfig.from.databaseName}.${chartConfig.from.tableName}.${keys.join(',')}.${chartConfig.dateRange.toString()}.${disableRowLimit}.${limit}.values`,packages/app/src/hooks/useMetadata.tsx (1)
174-179: Cache correctness: includelimitand the actual sliced keys in the React Query keyCurrently the queryKey excludes
limitand uses the unslicedkeys, while the request useskeys.slice(0, 50). This can lead to cache collisions and unnecessary cache fragmentation.- queryKey: [ - 'useMetadata.useGetKeyValues', - ...chartConfigsArr.map(cc => ({ ...cc })), - ...keys, - disableRowLimit, - ], + queryKey: [ + 'useMetadata.useGetKeyValues', + ...chartConfigsArr.map(cc => ({ ...cc })), + { keys: keys.slice(0, 50), limit, disableRowLimit }, + ],packages/app/src/components/SourceSelect.tsx (1)
45-46: Missing dependency in useMemo leads to stale labels
connectionsis used inside the memo but not listed as a dependency; labels won’t update when connections load/change.- [data, onCreate, allowedSourceKinds, hasLocalDefaultSources], + [data, onCreate, allowedSourceKinds, hasLocalDefaultSources, connections],packages/app/src/TeamPage.tsx (1)
595-613: Align “Delete Pending Invite” action with the same access gate used elsewhere.Currently this is gated only by
hasAdminAccess(true). Use the samecanAdmincheck you use for member removal to avoid exposing destructive actions to unauthorized users.- {hasAdminAccess && ( + {canAdmin && (packages/app/src/components/DBSearchPageFilters.tsx (1)
420-431: Prop should use SourceKind instead of string to prevent mismatches.Tighten typing to align with map and callers.
- sourceType, + sourceType, @@ - sourceType?: string; + sourceType?: SourceKind;packages/app/src/DBSearchPage.tsx (1)
1095-1103: Bug: Initial time search passes a single ISO string, which parseTimeQuery likely can’t parse.
onSearch(newTimeRange.toISOString())won’t match the expected “start - end” format (or a known token). This likely results in no-op or incorrect initial range.Use onTimeRangeSelect with the fixed 15-minute range and the display string:
- if (!searchParams.has('from') && !searchParams.has('to')) { - let newTimeRange = new Date(defaultTimeRange[0]); - onSearch(newTimeRange.toISOString()); - } + if (!searchParams.has('from') && !searchParams.has('to')) { + onTimeRangeSelect(defaultTimeRange[0], defaultTimeRange[1], initialTimeDisplay); + }Also, consider making
initialTimeDisplaysensitive to UTC if desired.
♻️ Duplicate comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
483-487: Use the unified keysToFetch in useGetKeyValues to avoid divergence.Avoid recomputing. This also helps when you later change fallback logic.
const keysToFetch = useMemo(() => { // Keep keys static relative to source type, independent of current selections return getKeysForSourceType(sourceType); }, [sourceType]); @@ } = useGetKeyValues({ chartConfigs: { ...chartConfig, dateRange }, limit: keyLimit, - keys: getKeysForSourceType(sourceType), + keys: keysToFetch, });Also applies to: 508-511
32-49: Override keys won’t match SourceKind; current map uses lowercase strings.
sourceTypeis passed asinputSourceObj?.kind(SourceKind). With lowercase keys (“log”, “trace”), overrides will never match, so no overrides apply.Apply this diff to align with the enum and ensure matches:
-import { ChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { ChartConfigWithDateRange, SourceKind } from '@hyperdx/common-utils/dist/types'; @@ -// Override keys for specific source types -const serviceMapOverride = { - log: [ +// Override keys for specific source types +const serviceMapOverride: Partial<Record<SourceKind, string[]>> = { + [SourceKind.Log]: [ 'SeverityText', 'ServiceName', "ResourceAttributes['k8s.cluster.name']", "ResourceAttributes['k8s.namespace.name']", ], - trace: [ + [SourceKind.Trace]: [ 'ServiceName', 'StatusCode', "ResourceAttributes['k8s.node.name']", "ResourceAttributes['k8s.owner.name']", 'SpanKind', ], - session: ['ServiceName', 'StatusCode', 'SpanKind'], - metric: ['ServiceName', 'MetricName', 'Unit'], + // Add other kinds when applicable, e.g. + // [SourceKind.Metric]: ['ServiceName', 'MetricName', 'Unit'], };
🧹 Nitpick comments (18)
packages/app/src/components/SelectControlled.tsx (1)
18-24: Tighten the selected predicate to avoid false positivesThe else branch returns true when an item lacks a value field, which can incorrectly treat the first such item as “selected”. Make the check strict.
- const selected = props.data?.find(d => - typeof d === 'string' - ? d === field.value - : 'value' in d - ? d.value === field.value - : true, - ); + const selected = props.data?.find(d => + typeof d === 'string' + ? d === field.value + : 'value' in d && d.value === field.value, + );packages/common-utils/src/metadata.ts (1)
451-451: Clamplimitto a safe range before embedding into SQLProtects against pathological values and keeps ClickHouse memory/response size in check.
async getKeyValues({ chartConfig, keys, - limit = 100, + limit = 100, disableRowLimit = false, }: { @@ - async () => { + async () => { + const safeLimit = Math.max(1, Math.min(500, limit ?? 100)); const sql = await renderChartConfig( { ...chartConfig, select: keys - .map((k, i) => `groupUniqArray(${limit})(${k}) AS param${i}`) + .map((k, i) => `groupUniqArray(${safeLimit})(${k}) AS param${i}`) .join(', '), }, this, );Also applies to: 461-468
packages/app/src/hooks/useMetadata.tsx (1)
193-197: Optional: extract and reuse the sliced keys for consistency and clarityKeeps queryKey and request in sync and makes the 50-key limit more discoverable.
- metadata.getKeyValues({ - chartConfig, - keys: keys.slice(0, 50), // Limit to 50 keys for now, otherwise request fails (max header size) - limit, - disableRowLimit, - }), + metadata.getKeyValues({ + chartConfig, + // Limit to 50 keys for now, otherwise request fails (max header size) + keys: keys.slice(0, 50), + limit, + disableRowLimit, + }),Consider hoisting
const slicedKeys = keys.slice(0, 50)above and using it in both queryKey and here.packages/app/src/components/SourceSelect.tsx (1)
34-35: Avoid “undefined | …” labels and satisfy Prettier by building label robustlyIf a connection name is not found, the current expression renders “undefined | …”. Build the label defensively.
- label: connections?.find(c => c.id === d.connection)?.name + ' | ' + d.name, + label: [connections?.find(c => c.id === d.connection)?.name, d.name] + .filter(Boolean) + .join(' | '),If you have many sources and connections, consider precomputing a Map for O(1) lookups:
const connectionNameById = useMemo(() => { const m = new Map<string, string>(); connections?.forEach(c => m.set(c.id, c.name)); return m; }, [connections]); // then label: [connectionNameById.get(d.connection), d.name].filter(Boolean).join(' | ')packages/app/src/TeamPage.tsx (4)
51-51: Sort imports to satisfy linter.ESLint (simple-import-sort/imports) flagged import order. Run autofix to keep CI green.
335-336: Compute a single canAdmin flag and reuse instead of repeatedly calling hasAccess.Minor readability and consistency improvement: derive
const canAdmin = hasAccess(me?.email)once and reuse across the component.Apply this diff at the declaration site:
const deleteTeamInvitation = api.useDeleteTeamInvitation(); - const { hasAccess } = useAuthEmails(); + const { hasAccess } = useAuthEmails(); + const canAdmin = hasAccess(me?.email);Then use
canAdminat call sites below.Also applies to: 299-306
554-571: Make the admin check consistent; drop redundant hasAdminAccess.
hasAdminAccessis hardcoded to true, so it’s redundant. Gate with the derivedcanAdmininstead for clarity.- {!member.isCurrentUser && hasAdminAccess && hasAccess(me?.email) && ( + {!member.isCurrentUser && canAdmin && (
554-554: Fix Prettier issues flagged by CI.
- Line 554: Prettier suggests breaking before
hasAdminAccess.- Line 1459: Remove the extra spaces on the blank line.
Run Prettier to resolve both.
Also applies to: 1459-1461
packages/app/src/hooks/useAuthEmails.tsx (3)
9-24: Strongly type the parsed value for safer access and better DX.Type
authArrayasRecord<string, boolean>and handle the case where someone sets the env to a string array of emails.-export const useAuthEmails = () => { - const authArray = useMemo(() => { +export const useAuthEmails = () => { + const authArray = useMemo<Record<string, boolean>>(() => { if (!HDX_AUTH_EMAILS) { console.warn('HDX_AUTH_EMAILS environment variable is not set'); - return {}; + return {}; } try { - const parsed = JSON.parse(HDX_AUTH_EMAILS); - return parsed; + const parsed = JSON.parse(HDX_AUTH_EMAILS); + if (Array.isArray(parsed)) { + return parsed.reduce((acc, email) => { + if (typeof email === 'string') acc[email] = true; + return acc; + }, {} as Record<string, boolean>); + } + return (parsed ?? {}) as Record<string, boolean>; } catch (error) { console.warn('Failed to parse HDX_AUTH_EMAILS:', error); return {}; } }, []);
41-42: Prettier formatting nit for filter callback.Trailing comma/newline suggested by Prettier. Run autofix.
1-2: Sort imports per linter.ESLint requests sorted imports.
packages/app/src/components/DBSearchPageFilters.tsx (3)
51-58: Type getKeysForSourceType with SourceKind and simplify.Use the enum in the signature and safe lookup with fallback.
-const getKeysForSourceType = (sourceType?: string) => { - if (sourceType && sourceType in serviceMapOverride) { - return serviceMapOverride[sourceType as keyof typeof serviceMapOverride]; - } - // For other source types, return empty array to use default behavior - return []; -}; +const getKeysForSourceType = (sourceType?: SourceKind): string[] => { + if (!sourceType) return []; + return serviceMapOverride[sourceType] ?? []; +};
435-465: Remove verbose debug console logging.The multiple console.error statements spam the console and may leak state. Keep only actionable error logs (e.g., in catch blocks) or guard with a debug flag.
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); - const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = usePinnedFilters(sourceId ?? null); - - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); - const { width, startResize } = useResizable(16, 'left'); - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); @@ - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); - const [showMoreFields, setShowMoreFields] = useState(false); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error('');Also applies to: 472-481, 461-463
540-541: Consider replacing console.error in catch with a user-visible notification.Optional: surface an alert when loading more facet values fails; console-only errors are easy to miss.
- } catch (error) { - console.error('failed to fetch more keys', error); + } catch (error: any) { + notifications.show({ + color: 'red', + message: 'Failed to fetch more filter values', + });packages/app/src/DBSearchPage.tsx (4)
533-542: Avoid empty catch block (no-empty).Add a small comment or a warn to satisfy the linter and future readers.
- }())(), + }())(), ); useEffect(() => { if (initialHadFiltersRef.current) { try { const url = new URL(window.location.href); url.searchParams.delete('filters'); window.history.replaceState({}, '', url.toString()); - } catch {} + } catch { + /* noop */ + } setSearchedConfig({ filters: [] }); } }, [setSearchedConfig]);
1264-1274: Gate the “Edit Source” gear icon using hasAccess instead of raw authArray lookup.Safer and consistent with the rest of the code; also avoids indexing with possibly undefined keys.
- {authArray[me?.email as keyof typeof authArray] && ( + {hasAccess(me?.email) && ( <Menu.Target>If you prefer, compute
const canAdmin = hasAccess(me?.email)once and use it here and elsewhere.Also applies to: 1275-1288
102-103: Fix Prettier complaint on multiline import.The linter suggests breaking the braced import into multiple lines with trailing commas.
521-542: Run Prettier on the file to resolve multiple formatting warnings.There are several Prettier hints (array/object trailing commas, spacing, and “prefer-const” at Line 1101). Running Prettier and minor fixes will get CI happy.
Also applies to: 676-681, 724-725, 794-804, 1101-1103, 1234-1242, 1264-1272
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/app/src/DBSearchPage.tsx(19 hunks)packages/app/src/TeamPage.tsx(7 hunks)packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)packages/app/src/components/SelectControlled.tsx(2 hunks)packages/app/src/components/SourceSelect.tsx(3 hunks)packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useAuthEmails.tsx(1 hunks)packages/app/src/hooks/useMetadata.tsx(1 hunks)packages/common-utils/src/metadata.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/app/src/components/SourceSelect.tsx (1)
packages/app/src/connection.ts (1)
useConnections(43-54)
packages/app/src/hooks/useAuthEmails.tsx (1)
packages/app/src/config.ts (1)
HDX_AUTH_EMAILS(14-16)
packages/app/src/TeamPage.tsx (1)
packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(8-49)
packages/app/src/DBSearchPage.tsx (5)
packages/app/src/connection.ts (1)
useConnections(43-54)packages/app/src/source.ts (1)
useSources(77-88)packages/app/src/timeQuery.ts (2)
dateRangeToString(38-48)useNewTimeQuery(432-530)packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(8-49)packages/app/src/components/SourceSelect.tsx (1)
SourceSelectControlled(64-64)
packages/app/src/components/DBSearchPageFilters.tsx (3)
packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (1)
useAllFields(82-126)
🪛 ESLint
packages/app/src/components/SourceSelect.tsx
[error] 34-34: Replace ·connections?.find(c·=>·c.id·===·d.connection)?.name·+·'·|·'·+ with ⏎············connections?.find(c·=>·c.id·===·d.connection)?.name·+⏎············'·|·'·+⏎···········
(prettier/prettier)
packages/app/src/config.ts
[error] 14-16: Replace ⏎··'NEXT_PUBLIC_HDX_AUTH_EMAILS',⏎ with 'NEXT_PUBLIC_HDX_AUTH_EMAILS'
(prettier/prettier)
packages/app/src/hooks/useAuthEmails.tsx
[error] 1-2: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 10-11: Delete ⏎····
(prettier/prettier)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 18-18: Unexpected console statement.
(no-console)
[error] 41-41: Replace email·=>·authArray[email·as·keyof·typeof·authArray] with ⏎······email·=>·authArray[email·as·keyof·typeof·authArray],⏎····
(prettier/prettier)
packages/app/src/TeamPage.tsx
[error] 1-51: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 554-554: Replace ·hasAdminAccess·&& with ⏎························hasAdminAccess·&&⏎·······················
(prettier/prettier)
[error] 1459-1459: Delete ··
(prettier/prettier)
packages/app/src/DBSearchPage.tsx
[error] 102-102: Replace ·parseTimeQuery,·useNewTimeQuery,·dateRangeToString· with ⏎··parseTimeQuery,⏎··useNewTimeQuery,⏎··dateRangeToString,⏎
(prettier/prettier)
[error] 539-539: Empty block statement.
(no-empty)
[error] 610-610: Delete ····
(prettier/prettier)
[error] 613-613: Replace s·=>·s.id·===·searchedConfig.source with ⏎········s·=>·s.id·===·searchedConfig.source,⏎······
(prettier/prettier)
[error] 618-618: Delete ····
(prettier/prettier)
[error] 621-621: Replace s·=>·s.id·===·lastSelectedSourceId with ⏎········s·=>·s.id·===·lastSelectedSourceId,⏎······
(prettier/prettier)
[error] 626-626: Delete ····
(prettier/prettier)
[error] 629-629: Replace searchedConfig.connection,·searchedConfig.source,·inputSourceObjs,·lastSelectedSourceId,·connections with ⏎····searchedConfig.connection,⏎····searchedConfig.source,⏎····inputSourceObjs,⏎····lastSelectedSourceId,⏎····connections,⏎··
(prettier/prettier)
[error] 676-676: Delete ··
(prettier/prettier)
[error] 678-680: Delete ⏎⏎
(prettier/prettier)
[error] 724-724: Replace ·searchedConfig?.connection·??·watch('connection')·?? with ⏎··········searchedConfig?.connection·??⏎··········watch('connection')·??⏎·········
(prettier/prettier)
[error] 794-794: Replace ·select,·where,·whereLanguage,·source,·filters,·orderBy,·connection with ⏎········select,⏎········where,⏎········whereLanguage,⏎········source,⏎········filters,⏎········orderBy,⏎········connection,⏎·····
(prettier/prettier)
[error] 858-858: Delete ······
(prettier/prettier)
[error] 1101-1101: 'newTimeRange' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 1234-1234: Insert ⏎·······
(prettier/prettier)
[error] 1238-1238: Replace s·=>·s.id·===·(currentSourceId·as·any) with ⏎······s·=>·s.id·===·(currentSourceId·as·any),⏎····
(prettier/prettier)
[error] 1240-1240: Replace ·shouldDirty:·true,·shouldTouch:·true with ⏎········shouldDirty:·true,⏎········shouldTouch:·true,⏎·····
(prettier/prettier)
[error] 1242-1243: Delete ⏎··
(prettier/prettier)
[error] 1264-1264: Insert ·
(prettier/prettier)
[error] 1265-1265: Insert ········
(prettier/prettier)
[error] 1266-1266: Insert ··
(prettier/prettier)
[error] 1267-1267: Insert ··
(prettier/prettier)
[error] 1268-1268: Insert ··
(prettier/prettier)
[error] 1269-1269: Replace ·············· with ················
(prettier/prettier)
[error] 1270-1270: Insert ··
(prettier/prettier)
[error] 1271-1271: Insert ··
(prettier/prettier)
[error] 1272-1272: Insert ··
(prettier/prettier)
🔇 Additional comments (6)
packages/app/src/components/SelectControlled.tsx (1)
15-17: Nice: defers clearing until options are ready to prevent flickerThe hasOptions + conditional value binding is a solid way to avoid premature clears while options load. LGTM.
Also applies to: 41-41
packages/app/src/TeamPage.tsx (2)
1482-1488: LGTM: Sections correctly gated by hasAccess.Rendering Sources, Connections, TeamQueryConfig, and API Keys only for authorized users looks correct.
1052-1063: LGTM: Button-level gating is appropriate here.Team name is visible to all, but “Change” action is restricted via
hasAccess, which keeps UX friendly without over-gating.packages/app/src/components/DBSearchPageFilters.tsx (2)
205-210: Auto “load more” on mount may surprise users; ensure it’s desired.The effect eagerly calls
onLoadMore(name)once per group. If this is meant to be lazy, keep it; otherwise consider deferring to user action only.Would you like me to switch this to only trigger on user click?
276-276: Good: included isFieldPinned in dependencies.The memo recomputes when pin status changes; avoids stale ordering.
packages/app/src/DBSearchPage.tsx (1)
399-405: Good UX: default to a fixed 15-minute range and disable live tail initially.The fixed window and live-tail toggle behaviors are coherent and prevent unexpected auto-refreshes.
Also applies to: 673-688
packages/app/src/config.ts
Outdated
| export const HDX_AUTH_EMAILS = env( | ||
| 'NEXT_PUBLIC_HDX_AUTH_EMAILS', | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix Prettier error and add a safe default to prevent JSON.parse crashes downstream
Prettier flags the multi-line arg. Also, if this env is unset, consumers that JSON.parse it will crash. Inline the arg and default to '[]'.
Apply this diff:
-export const HDX_AUTH_EMAILS = env(
- 'NEXT_PUBLIC_HDX_AUTH_EMAILS',
-);
+export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS') ?? '[]';Follow-up: confirm the client hook parsing HDX_AUTH_EMAILS tolerates malformed JSON and treats empty/missing as an empty list to avoid runtime errors and UX regressions.
🏁 Script executed:
#!/bin/bash
# Inspect imports and env usage in config.ts
sed -n '1,40p' packages/app/src/config.ts
# Search for HDX_AUTH_EMAILS usage across the codebase
rg -n --color=never "HDX_AUTH_EMAILS" -C2Length of output: 4569
Update HDX_AUTH_EMAILS default and formatting
• In packages/app/src/config.ts (lines 14–16), replace the multi-line env call with an inline invocation that falls back to '[]':
-export const HDX_AUTH_EMAILS = env(
- 'NEXT_PUBLIC_HDX_AUTH_EMAILS',
-);
+export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS') ?? '[]';This satisfies Prettier, ensures JSON.parse never receives undefined, and guarantees an empty array by default.
• Verify in packages/app/src/hooks/useAuthEmails.tsx that the try/catch around JSON.parse will handle any malformed JSON without crashing and, with the new default, always produce [].
📝 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.
| export const HDX_AUTH_EMAILS = env( | |
| 'NEXT_PUBLIC_HDX_AUTH_EMAILS', | |
| ); | |
| export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS') ?? '[]'; |
🧰 Tools
🪛 ESLint
[error] 14-16: Replace ⏎··'NEXT_PUBLIC_HDX_AUTH_EMAILS',⏎ with 'NEXT_PUBLIC_HDX_AUTH_EMAILS'
(prettier/prettier)
🤖 Prompt for AI Agents
In packages/app/src/config.ts around lines 14 to 16, replace the multi-line env
call with a single-line invocation that provides a default string '[]' (e.g.
env('NEXT_PUBLIC_HDX_AUTH_EMAILS', '[]')) so Prettier is satisfied and
JSON.parse never receives undefined; then verify
packages/app/src/hooks/useAuthEmails.tsx still wraps JSON.parse in a try/catch
to gracefully handle malformed JSON and ensure the hook returns an empty array
by default with the new config fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/DBSearchPage.tsx (1)
1106-1114: Bug: Initial search sends an ISO string instead of a time range.onSearch expects a time range string (e.g., "start - end") or use onTimeRangeSelect. Passing a single ISO date will not parse into from/to and can yield incorrect behavior.
Apply this diff to fix the initialization:
- if (!searchParams.has('from') && !searchParams.has('to')) { - const newTimeRange = new Date(defaultTimeRange[0]); - onSearch(newTimeRange.toISOString()); - } + if (!searchParams.has('from') && !searchParams.has('to')) { + onTimeRangeSelect(defaultTimeRange[0], defaultTimeRange[1], initialTimeDisplay); + }
♻️ Duplicate comments (3)
packages/app/src/DBSearchPage.tsx (1)
1581-1582: Passing SourceKind to DBSearchPageFilters is correct and aligns with the earlier review.Prop now receives inputSourceObj?.kind as SourceKind. Good follow-through.
packages/app/src/TeamPage.tsx (2)
400-435: Same refactor applies here (reuse showKyError)
444-477: Same refactor applies here (reuse showKyError)
🧹 Nitpick comments (11)
packages/app/src/DBSearchPage.tsx (4)
600-602: Avoid fetching sources twice; reuse the earlier useSources() result.You already have const { data: sources } = useSources(). Calling useSources() again will create an extra hook (React Query dedupes network calls, but this is unnecessary churn and cognitive load).
Replace the second call with the existing data:
- const { data: inputSourceObjs } = useSources(); + const inputSourceObjs = sources;
682-688: Prevent potential isLive flicker at startup.This effect sets isLive to false for fixed ranges, but the subsequent init effect that sets live mode when from/to are null can briefly flip it to true. Low impact, but visible UI flicker is possible.
One simple mitigation: in the “auto set live” effect, only enable live mode when the displayedTimeInputValue equals 'Live Tail' (or when it’s not the fixed initial display). Alternatively, rely solely on the corrected initial onTimeRangeSelect (see prior comment), which will populate from/to immediately and avoid the toggle.
1221-1222: Use hasAccess() from useAuthEmails to encapsulate auth logic.Directly indexing into authArray is brittle; use the provided helper.
- const { authArray } = useAuthEmails(); + const { hasAccess } = useAuthEmails();
1290-1304: Gate admin UI with hasAccess() instead of object indexing.This avoids optional chaining edge cases and centralizes the auth logic.
- {authArray[me?.email as keyof typeof authArray] && ( + {hasAccess(me?.email) && ( <Menu.Target> <ActionIcon variant="subtle" color="dark.2" size="sm" title="Edit Source" > <Text size="xs"> <i className="bi bi-gear" /> </Text> </ActionIcon> </Menu.Target> )}packages/app/src/TeamPage.tsx (7)
298-298: Minor: avoid redundant me queries across componentsapi.useMe() is called in TeamPage, TeamMembersSection, TeamNameSection, and ApiKeysSection. React Query will dedupe/cache, but you can avoid extra hook invocations by passing me down as a prop or via a light context if this grows. Low impact; optional polish.
349-376: DRY up HTTPError notification handlingThe onError branch duplicates a ky HTTPError parsing pattern across actions. Extract a small helper to centralize JSON parsing + fallback messaging to reduce duplication and keep behavior consistent.
Apply this diff to this onError:
- onError: (e: any) => { - if (e instanceof HTTPError) { - e.response - .json() - .then((res: any) => { - notifications.show({ - color: 'red', - message: res.message, - autoClose: 5000, - }); - }) - .catch(() => { - notifications.show({ - color: 'red', - message: - 'Something went wrong. Please contact HyperDX team.', - autoClose: 5000, - }); - }); - } else { - notifications.show({ - color: 'red', - message: 'Something went wrong. Please contact HyperDX team.', - autoClose: 5000, - }); - } - }, + onError: e => showKyError(e, 'Something went wrong. Please contact HyperDX team.'),Add this helper near the top of the file (outside these ranges):
function showKyError(e: unknown, fallback = 'Something went wrong. Please contact HyperDX team.') { if (e instanceof HTTPError) { e.response .json() .then((res: any) => { notifications.show({ color: 'red', message: res?.message ?? fallback, autoClose: 5000, }); }) .catch(() => { notifications.show({ color: 'red', message: fallback, autoClose: 5000, }); }); } else { const message = (e as any)?.message ?? fallback; notifications.show({ color: 'red', message, autoClose: 5000, }); } }
968-972: TeamNameSection: ensure form default value updates when team loads
- defaultValues uses team.name but the hook may render before team is available; RHF won’t update defaultValues after mount, so the input may stay empty until a manual reset.
- Also, api.useMe() is invoked again here; consider passing me from parent if convenient (optional).
Suggested fix to keep the form in sync:
- const form = useForm<{ name: string }>({ - defaultValues: { - name: team.name, - }, - }); + const form = useForm<{ name: string }>({ + defaultValues: { + name: team?.name ?? '', + }, + }); + + useEffect(() => { + if (team?.name != null) { + form.reset({ name: team.name }); + } + }, [team?.name, form]);
1132-1146: LGTM: ClickHouse settings mutate handlerStraightforward success/failure UX. Consider adopting the same showKyError helper for consistency across mutations, but not required.
1349-1350: Minor: avoid another useMe() hereYou can pass me down from TeamPage to ApiKeysSection to save another hook call. Low priority due to caching.
1461-1463: Compute canManage once and reuse in JSXYou call hasAccess(me?.email) multiple times. Compute a single boolean to avoid repetition and make conditions easier to scan.
- // Use auth emails hook - const { hasAccess } = useAuthEmails(); + // Use auth emails hook + const { hasAccess } = useAuthEmails(); + const canManage = hasAccess(me?.email);
1483-1489: Use the computed canManage for gated sectionsSmall readability improvement; behavior unchanged.
- {hasAccess(me?.email) && <SourcesSection />} - {hasAccess(me?.email) && <ConnectionsSection />} + {canManage && <SourcesSection />} + {canManage && <ConnectionsSection />} <IntegrationsSection /> <TeamNameSection /> - {hasAccess(me?.email) && <TeamQueryConfigSection />} - {hasAccess(me?.email) && <ApiKeysSection />} + {canManage && <TeamQueryConfigSection />} + {canManage && <ApiKeysSection />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/app/src/DBSearchPage.tsx(23 hunks)packages/app/src/TeamPage.tsx(15 hunks)packages/app/src/components/SourceSelect.tsx(3 hunks)packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useAuthEmails.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/config.ts
- packages/app/src/components/SourceSelect.tsx
- packages/app/src/hooks/useAuthEmails.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/app/src/DBSearchPage.tsx (4)
packages/app/src/connection.ts (1)
useConnections(43-54)packages/app/src/source.ts (1)
useSources(77-88)packages/app/src/timeQuery.ts (2)
dateRangeToString(38-48)useNewTimeQuery(432-530)packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)
packages/app/src/TeamPage.tsx (1)
packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)
🔇 Additional comments (13)
packages/app/src/DBSearchPage.tsx (8)
518-542: Stripping 'filters' from the URL on initial load: confirm UX is intentional.This proactively removes filters from the URL and resets in-memory filters. It makes shared links non-idempotent on first visit and may surprise users who expect the exact filters to be applied when opening/sharing a link.
- If the intent is to avoid initial narrowing for facets, consider gating this behind a feature flag, applying it only when landing without a saved search, or limiting it to specific routes.
- Alternatively, only ignore filters for the facets’ initial fetch without mutating the URL (so that a refresh preserves the URL as shared).
395-402: Fixed default time range is clear and safe with SSR disabled.The intent and implementation look good. Module-level initialization is fine given ssr: false.
507-508: Persisting 'connection' in URL state is a good addition.Including connection in queryStateMap ensures deep-linking and back/forward navigation consistency.
604-637: Initial connection derivation logic is sensible.Preferring URL > current source > last-selected source > first available connection covers the common cases.
639-658: Form schema/type alignment for 'connection' looks correct.Schema, type, and form initialization are consistent. The follow-up effect that sets connection when data arrives closes the loop.
917-926: Ignoring initial filters for the first render is a pragmatic way to avoid narrowed facets.The searchedConfigForInit approach cleanly decouples UI init from persisted state.
1240-1260: Auto-selecting a source that matches the selected connection is helpful.This improves UX when connections change. Since your source-change watcher already clears filters and triggers refresh, the state remains consistent.
1280-1289: Min-width wrapper around SourceSelectControlled improves layout stability.Small UI enhancement that prevents jitter. Looks good.
packages/app/src/TeamPage.tsx (5)
553-572: LGTM: removal action gated behind allowlistGood call gating destructive actions behind hasAccess. Pair this with backend authorization.
985-991: Simplified onError handler is fineMoving to onError: () => ... keeps the UX consistent and avoids leaking raw error details. No issues.
1053-1064: LGTM: “Change team name” CTA gated by allowlistConsistent with the rest of the page’s admin-only gating.
1444-1452: LGTM: Personal API key visibility tied to me presenceReasonable default; shows only for the current user context.
43-43: Confirm public env var for HDX_AUTH_EMAILS is wired correctly
- In packages/app/src/config.ts, you export
export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS')- Next.js will inline any
NEXT_PUBLIC_*vars into both server and client bundles at build time—but you must still ensure that:
• Your local.env.localand production/CI environments defineNEXT_PUBLIC_HDX_AUTH_EMAILSwith the expected JSON string.
• If you’ve locked down env vars innext.config.js, that key is whitelisted.- Without it,
useAuthEmails()returns{}and you won’t see an exception—but mismatched or missing values between builds can cause unexpected behavior.(Optional) consider adding a runtime warning or fallback so you’re alerted if the JSON is missing or invalid.
| const { hasAccess } = useAuthEmails(); | ||
|
|
||
| const sendTeamInviteAction = (email: string) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Apply hasAccess consistently to invites and pending-invite deletions; consolidate the check
You already gate the Remove action; the “Invite Team Member” and “Delete (pending invite)” actions are still available to everyone. Align these to avoid bypassing your allowlist at the UI layer. Also, define a single canManageMembers variable to reduce repetition and improve readability.
Within the changed lines, add a consolidated flag:
- const { hasAccess } = useAuthEmails();
+ const { hasAccess } = useAuthEmails();
+ const canManageMembers = hasAdminAccess && hasAccess(me?.email);Within the changed block for the member Remove button:
- {!member.isCurrentUser &&
- hasAdminAccess &&
- hasAccess(me?.email) && (
+ {!member.isCurrentUser && canManageMembers && (
<Group justify="flex-end" gap="8">
<Button
size="compact-sm"
variant="light"
color="red"
onClick={() =>
setDeleteTeamMemberConfirmationModalData({
mode: 'team',
id: member._id,
email: member.email,
})
}
>
Remove
</Button>
</Group>
)}Additionally apply gating to unchanged spots outside this hunk:
- Gate “Invite Team Member”:
- <Button
- variant="light"
- leftSection={<i className="bi bi-person-plus-fill" />}
- onClick={() => setTeamInviteModalShow(true)}
- >
- Invite Team Member
- </Button>
+ {canManageMembers && (
+ <Button
+ variant="light"
+ leftSection={<i className="bi bi-person-plus-fill" />}
+ onClick={() => setTeamInviteModalShow(true)}
+ >
+ Invite Team Member
+ </Button>
+ )}- Gate “Delete (pending invite)”:
- {hasAdminAccess && (
+ {canManageMembers && (
<Group justify="flex-end" gap="8">
<Button
size="compact-sm"
variant="light"
color="red"
onClick={() =>
setDeleteTeamMemberConfirmationModalData({
mode: 'teamInvite',
id: invitation._id,
email: invitation.email,
})
}
>
Delete
</Button>
</Group>
)}Note: UI gating is not a security boundary. Ensure the backend endpoints for invite/delete are also enforcing this allowlist (email-based gating).
🏁 Script executed:
#!/bin/bash
set -e
echo "1) useAuthEmails import and usage:"
grep -R "useAuthEmails" -n packages/app/src/TeamPage.tsx || true
echo -e "\n2) hasAdminAccess usage:"
rg -n --type=tsx "hasAdminAccess" packages/app/src/TeamPage.tsx -C2 || true
echo -e "\n3) hasAccess usage:"
rg -n --type=tsx "hasAccess" packages/app/src/TeamPage.tsx -C2 || true
echo -e "\n4) Invite Team Member button location:"
rg -n "Invite Team Member" packages/app/src/TeamPage.tsx -C3 || true
echo -e "\n5) Pending invite delete button (mode 'teamInvite'):"
rg -n "teamInvite" packages/app/src/TeamPage.tsx -C3 || trueLength of output: 2959
🏁 Script executed:
#!/bin/bash
set -e
echo "1) hasAdminAccess usages around line 330:"
grep -R "hasAdminAccess" -n packages/app/src/TeamPage.tsx -C3 | sed -n '1,20p'
echo -e "\n2) Pending-invite delete button context:"
grep -R "invitation" -n packages/app/src/TeamPage.tsx -C3 | sed -n '580,650p'Length of output: 1081
Consolidate UI checks for team member management and gate invite/delete actions with canManageMembers
In packages/app/src/TeamPage.tsx, inside TeamMembersSection:
- After retrieving
hasAccessfromuseAuthEmails, define a single flag:const { hasAccess } = useAuthEmails(); +const canManageMembers = hasAdminAccess && hasAccess(me?.email); - Replace the “Remove” button conditional (around line 553):
-{!member.isCurrentUser && hasAdminAccess && hasAccess(me?.email) && ( +{!member.isCurrentUser && canManageMembers && (
- Wrap the “Invite Team Member” button (around line 499):
-<Button - variant="light" - leftSection={<i className="bi bi-person-plus-fill" />} - onClick={() => setTeamInviteModalShow(true)} -> +{canManageMembers && ( + <Button + variant="light" + leftSection={<i className="bi bi-person-plus-fill" />} + onClick={() => setTeamInviteModalShow(true)} + > + Invite Team Member + </Button> +)}
- Update the pending-invite “Delete” button conditional (around line 601):
-{hasAdminAccess && ( +{canManageMembers && (
Note: UI gating is not a security boundary—verify that your backend invite/delete endpoints also enforce this email-based allowlist.
📝 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.
| const { hasAccess } = useAuthEmails(); | |
| const sendTeamInviteAction = (email: string) => { | |
| const { hasAccess } = useAuthEmails(); | |
| const canManageMembers = hasAdminAccess && hasAccess(me?.email); | |
| const sendTeamInviteAction = (email: string) => { |
🤖 Prompt for AI Agents
In packages/app/src/TeamPage.tsx around lines 334-336, define a single boolean
flag const canManageMembers = hasAccess('team:manage-members') (or equivalent
access key) immediately after extracting hasAccess; then update the UI checks to
use this flag: replace the conditional that shows the “Remove” button (around
line 553) to check canManageMembers, wrap the “Invite Team Member” button
(around line 499) with canManageMembers, and change the pending-invite “Delete”
button conditional (around line 601) to use canManageMembers as well so all
invite/delete/manage-member UI is gated by the same flag.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/DBSearchPage.tsx (2)
1106-1114: Bug: Initial onSearch sends a single ISO, not a range (parser likely expects a range).This calls onSearch with only the start ISO timestamp. useNewTimeQuery.onSearch expects a parsable time query (e.g., "start - end" or "Live Tail"). Sending a single ISO will likely fail parseTimeQuery and keep from/to unset, which can flip isLive or prevent initial data load.
Apply this diff to set the initial range correctly:
- if (!searchParams.has('from') && !searchParams.has('to')) { - const newTimeRange = new Date(defaultTimeRange[0]); - onSearch(newTimeRange.toISOString()); - } + if (!searchParams.has('from') && !searchParams.has('to')) { + onTimeRangeSelect( + defaultTimeRange[0], + defaultTimeRange[1], + initialTimeDisplay, + ); + }
1068-1086: Include the connection parameter and guard against undefined filters in generateSearchUrlThe current
generateSearchUrlomitsconnectionand always serializesfilters, which can producefilters=undefinedor drop the user’s connection selection when sharing URLs. This breaks reproducibility and may run queries against the wrong connection.Apply this refactor around lines ~1068–1086 in
packages/app/src/DBSearchPage.tsx:- }) => { - const qParams = new URLSearchParams({ - where: where || searchedConfig.where || '', - whereLanguage: whereLanguage || 'sql', - from: searchedTimeRange[0].getTime().toString(), - to: searchedTimeRange[1].getTime().toString(), - select: searchedConfig.select || '', - source: searchedSource?.id || '', - filters: JSON.stringify(searchedConfig.filters), - }); - return `/search?${qParams.toString()}`; - }, + }) => { + const qParams = new URLSearchParams(); + qParams.set('where', where || searchedConfig.where || ''); + qParams.set('whereLanguage', whereLanguage || 'sql'); + qParams.set('from', searchedTimeRange[0].getTime().toString()); + qParams.set('to', searchedTimeRange[1].getTime().toString()); + qParams.set('select', searchedConfig.select || ''); + qParams.set('source', searchedSource?.id || ''); + // Only include filters when present to avoid filters=undefined/null + if (searchedConfig.filters?.length) { + qParams.set('filters', JSON.stringify(searchedConfig.filters)); + } + // Preserve the selected connection in the URL + if (searchedConfig.connection) { + qParams.set('connection', searchedConfig.connection); + } + return `/search?${qParams.toString()}`; + },
- Adds the missing
connectionquery parameter.- Guards
filtersto preventfilters=undefinedfrom appearing.
🧹 Nitpick comments (10)
packages/app/src/TeamPage.tsx (7)
493-501: Also gate “Invite Team Member” and “Delete pending invite” behind hasAccessRight now, Invite and Delete-invite actions are only guarded by hasAdminAccess. For consistency with the new email-allowlist gate (hasAccess), these should be gated as well to avoid exposing sensitive admin actions to users not in HDX_AUTH_EMAILS.
Apply these diffs:
--- a/packages/app/src/TeamPage.tsx +++ b/packages/app/src/TeamPage.tsx @@ - <Button - variant="light" - leftSection={<i className="bi bi-person-plus-fill" />} - onClick={() => setTeamInviteModalShow(true)} - > - Invite Team Member - </Button> + {hasAdminAccess && hasAccess(me?.email) && ( + <Button + variant="light" + leftSection={<i className="bi bi-person-plus-fill" />} + onClick={() => setTeamInviteModalShow(true)} + > + Invite Team Member + </Button> + )}--- a/packages/app/src/TeamPage.tsx +++ b/packages/app/src/TeamPage.tsx @@ - {hasAdminAccess && ( + {hasAdminAccess && hasAccess(me?.email) && ( <Group justify="flex-end" gap="8"> <Button size="compact-sm" variant="light" color="red"Also applies to: 596-614
1461-1463: Reduce repetition: compute canManage once and reuseYou call hasAccess(me?.email) multiple times. Compute once for readability and to avoid redundant evaluations.
@@ - // Use auth emails hook - const { hasAccess } = useAuthEmails(); + // Use auth emails hook + const { hasAccess } = useAuthEmails(); + const canManage = hasAccess(me?.email); @@ - {hasAccess(me?.email) && <SourcesSection />} - {hasAccess(me?.email) && <ConnectionsSection />} + {canManage && <SourcesSection />} + {canManage && <ConnectionsSection />} @@ - {hasAccess(me?.email) && <TeamQueryConfigSection />} - {hasAccess(me?.email) && <ApiKeysSection />} + {canManage && <TeamQueryConfigSection />} + {canManage && <ApiKeysSection />}Also applies to: 1483-1484, 1487-1488
1390-1396: Use distinct data-test-id values to avoid selector collisionsBoth team API key and personal API key use dataTestId="api-key". This can make E2E/DOM tests flaky/ambiguous.
@@ - <APIKeyCopyButton value={team.apiKey} dataTestId="api-key" /> + <APIKeyCopyButton value={team.apiKey} dataTestId="team-api-key" /> @@ - <APIKeyCopyButton value={me.accessKey} dataTestId="api-key" /> + <APIKeyCopyButton value={me.accessKey} dataTestId="personal-api-key" />Also applies to: 1447-1451
1443-1452: Guard on me.accessKey existence before rendering Personal API keyIf me exists but accessKey is missing/undefined, the prop type contract for APIKeyCopyButton (value: string) is violated.
- {me != null && ( + {Boolean(me?.accessKey) && ( <Card> <Card.Section p="md"> <Text c="gray.3" mb="md"> Personal API Access Key </Text> - <APIKeyCopyButton value={me.accessKey} dataTestId="api-key" /> + <APIKeyCopyButton value={me!.accessKey as string} dataTestId="personal-api-key" /> </Card.Section> </Card> )}Note: Alternatively, tighten the me type upstream so accessKey is always present for authenticated users.
341-376: Deduplicate HTTPError notification handling into a small helperThe onError branches repeat the same HTTPError parsing and fallback logic. Centralize to reduce duplication and keep behavior consistent.
Add a small helper (near imports or in a shared utils file):
function showApiError(e: unknown, fallback: string) { if (e instanceof HTTPError) { e.response .json() .then((res: any) => { notifications.show({ color: 'red', message: res?.message ?? fallback, autoClose: 5000, }); }) .catch(() => { notifications.show({ color: 'red', message: fallback, autoClose: 5000 }); }); } else { notifications.show({ color: 'red', message: (e as any)?.message ?? fallback, autoClose: 5000 }); } }Then replace in onError callbacks, e.g.:
- onError: (e: any) => { - if (e instanceof HTTPError) { - e.response - .json() - .then((res: any) => { - notifications.show({ - color: 'red', - message: res.message, - autoClose: 5000, - }); - }) - .catch(() => { - notifications.show({ - color: 'red', - message: - 'Something went wrong. Please contact HyperDX team.', - autoClose: 5000, - }); - }); - } else { - notifications.show({ - color: 'red', - message: 'Something went wrong. Please contact HyperDX team.', - autoClose: 5000, - }); - } - }, + onError: (e: unknown) => + showApiError(e, 'Something went wrong. Please contact HyperDX team.'),Same simplification can be applied to deleteTeamInviteAction, deleteTeamMemberAction, rotateTeamApiKeyAction, and similar onError sites.
Also applies to: 400-436, 444-479, 985-999, 1132-1146, 1365-1371
968-972: Ensure form defaults update if team changes (edge case)If team.name updates while this component is mounted (e.g., switching teams via cache warm), defaultValues won’t reinitialize. Consider resetting the form when team.name changes.
You can add:
useEffect(() => { form.reset({ name: team?.name ?? '' }); }, [team?.name]);
298-305: Avoid redundant useMe/useTeam calls across nested componentsYou call api.useMe()/api.useTeam() in multiple sibling/child components. React Query will cache responses, but you still incur extra hook invocations and render churn. Consider lifting me/team to TeamPage and passing down via context/props where practical.
Also applies to: 968-972, 1348-1352, 1458-1463
packages/app/src/DBSearchPage.tsx (3)
395-402: Avoid module-evaluated default time range (stale on hot reload / long sessions).defaultTimeRange is computed once at module load. In long-lived sessions or hot reload, this baseline drifts from user expectations of "last 15 minutes" at page open.
Consider computing it per-mount:
-const defaultTimeRange = createFixedTimeRange();Then inside the DBSearchPage component (near other initializations):
// inside DBSearchPage const defaultTimeRange = useMemo(() => createFixedTimeRange(), []);This keeps your "fixed 15m" invariant per mount without drifting across reloads.
1220-1222: Use hasAccess() from useAuthEmails for clarity and safety.Directly indexing authArray with me?.email is brittle; hasAccess already encapsulates parsing and guards.
Apply this diff:
- const { authArray } = useAuthEmails(); + const { hasAccess } = useAuthEmails();- {authArray[me?.email as keyof typeof authArray] && ( + {hasAccess(me?.email) && ( <Menu.Target> <ActionIcon variant="subtle" color="dark.2" size="sm" title="Edit Source" > <Text size="xs"> <i className="bi bi-gear" /> </Text> </ActionIcon> </Menu.Target> )}Also applies to: 1291-1303
462-497: Unify return shape of useSearchedConfigToChartConfig.When sourceObj exists, the hook returns { data } without isLoading; else it returns { data: null, isLoading }. This leads to isLoading being boolean | undefined downstream. Prefer consistent shape.
Apply this diff:
- return useMemo(() => { + return useMemo(() => { if (sourceObj != null) { return { - data: { + data: { select: select || (sourceObj.defaultTableSelectExpression ?? ''), from: sourceObj.from, ...(sourceObj.tableFilterExpression != null ? { filters: [ { type: 'sql' as const, condition: sourceObj.tableFilterExpression, }, ...(filters ?? []), ], } : {}), ...(filters != null ? { filters } : {}), where: where ?? '', whereLanguage: whereLanguage ?? 'sql', timestampValueExpression: sourceObj.timestampValueExpression, implicitColumnExpression: sourceObj.implicitColumnExpression, connection: sourceObj.connection, displayType: DisplayType.Search, orderBy: orderBy || `${getFirstTimestampValueExpression( sourceObj.timestampValueExpression, )} DESC`, - }, + }, + isLoading, }; } - return { data: null, isLoading }; + return { data: null, isLoading }; }, [sourceObj, isLoading, select, filters, where, whereLanguage, orderBy]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/app/src/DBSearchPage.tsx(23 hunks)packages/app/src/TeamPage.tsx(15 hunks)packages/app/src/components/SourceSelect.tsx(3 hunks)packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useAuthEmails.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/components/SourceSelect.tsx
- packages/app/src/config.ts
- packages/app/src/hooks/useAuthEmails.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/app/src/TeamPage.tsx (1)
packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)
packages/app/src/DBSearchPage.tsx (5)
packages/app/src/connection.ts (1)
useConnections(43-54)packages/app/src/source.ts (1)
useSources(77-88)packages/app/src/timeQuery.ts (2)
dateRangeToString(38-48)useNewTimeQuery(432-530)packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)packages/app/src/components/SourceSelect.tsx (1)
SourceSelectControlled(67-67)
🔇 Additional comments (6)
packages/app/src/TeamPage.tsx (4)
43-43: Import of useAuthEmails is correct and scoped wellThe hook location and relative path look good. No concerns here.
553-572: Good: remove-member action is now gated by authorized emailCombining not-current-user, admin access, and hasAccess(me?.email) is sensible and consistent with the auth hook intent.
1053-1064: Team name “Change” control gated by hasAccess: looks goodThe admin + email-allowlist check aligns with the intended access restrictions.
1461-1463: Confirm deny-by-default behavior for hasAccess is intentionalWe’ve verified that HDX_AUTH_EMAILS is loaded from
NEXT_PUBLIC_HDX_AUTH_EMAILSviaenv()inpackages/app/src/config.ts, and no.envfile defines it by default. As a result,useAuthEmailsreturnshasAccess = falsewhenever the variable is missing or invalid—hiding all gated UI.• If this gating is intended as a feature-flag or temporary allowlist, no changes are needed.
• Otherwise, consider providing a permissive default in dev/staging, for example:
– DefineNEXT_PUBLIC_HDX_AUTH_EMAILSin your.env.developmentor.env.local.
– Or adjustuseAuthEmailsto default totrueoutside production, e.g.:
const hasAccess = IS_PROD ? emails.length > 0 : true;packages/app/src/DBSearchPage.tsx (2)
518-542: Stripping filters from URL on first load: confirm desired UX and history behavior.This mutates the URL and resets in-memory filters on mount if filters was present. If users share URLs including filters, the page will discard them on first open, which may be surprising. Also note history.replaceState removes the param without leaving a back entry.
If intentional (as per PR summary to avoid narrowing facets), consider:
- Showing a one-time toast hinting that filters were cleared.
- Gating behind a feature flag or only applying on landing routes (not internal navigations).
No action requested if this is by design—flagging for confirmation.
1581-1582: Propagating SourceKind to filters: LGTM.Passing sourceType={inputSourceObj?.kind} aligns with the prior recommendation to type this prop as SourceKind in DBSearchPageFilters. Ensure the filters component prop and helper typings are updated accordingly.
| useEffect(() => { | ||
| if (!selectedConnectionId || !inputSourceObjs) return; | ||
| const allowedKinds = [SourceKind.Log, SourceKind.Trace]; | ||
| const firstMatching = inputSourceObjs.find( | ||
| s => | ||
| s.connection === selectedConnectionId && allowedKinds.includes(s.kind), | ||
| ); | ||
| if (!firstMatching) return; | ||
| const currentSourceId = watch('source'); | ||
| const currentSource = inputSourceObjs.find( | ||
| s => s.id === (currentSourceId as any), | ||
| ); | ||
| if (!currentSource || currentSource.connection !== selectedConnectionId) { | ||
| setValue('source', firstMatching.id, { | ||
| shouldDirty: true, | ||
| shouldTouch: true, | ||
| }); | ||
| } | ||
| }, [selectedConnectionId, inputSourceObjs, setValue, watch]); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Programmatic source change doesn't apply source defaults (select/orderBy) or clear filters.
The watch handler at Lines 842-869 only fires its reset logic on type === 'change', which may not trigger when you setValue('source', ...) programmatically in the connection effect (Lines 1241-1260). Result: source updates via connection may leave select/orderBy/filters stale.
Two options:
- Replicate the same reset logic inside the connection effect after setValue('source', ...).
- Or extract a helper (applySourceDefaults) and call it from both places.
Minimal patch replicated in the connection effect:
if (!currentSource || currentSource.connection !== selectedConnectionId) {
setValue('source', firstMatching.id, {
shouldDirty: true,
shouldTouch: true,
});
+ const newInputSourceObj = inputSourceObjs.find(s => s.id === firstMatching.id);
+ if (newInputSourceObj) {
+ setLastSelectedSourceId(newInputSourceObj.id);
+ setValue(
+ 'select',
+ newInputSourceObj.defaultTableSelectExpression ?? '',
+ );
+ setValue(
+ 'orderBy',
+ `${getFirstTimestampValueExpression(
+ newInputSourceObj.timestampValueExpression ?? '',
+ )} DESC`,
+ );
+ searchFilters.clearAllFilters();
+ setTimeout(() => debouncedSubmit(), 0);
+ }
}Remember to include missing deps in the effect: setLastSelectedSourceId, searchFilters, debouncedSubmit.
Also applies to: 842-869
🏁 Script executed:
#!/bin/bash
# Display lines around the watch handler (Lines ~842-869)
sed -n '830,900p' packages/app/src/DBSearchPage.tsxLength of output: 2168
🏁 Script executed:
#!/bin/bash
# Show the top imports to see how watch is obtained
sed -n '1,200p' packages/app/src/DBSearchPage.tsxLength of output: 5564
🏁 Script executed:
#!/bin/bash
# Search for definitions of inputSourceObj(s)
rg -n "inputSourceObj" -n "inputSourceObjs" -C3 packages/app/src/DBSearchPage.tsxLength of output: 9372
🌐 Web query:
In react-hook-form, does calling setValue trigger the watch subscription callback, and if so, what is the 'type' parameter passed (e.g., 'change')?
💡 Result:
Short answer: Yes — programmatic setValue updates will (in most cases) invoke the callback-style watch, but the event "type" is NOT "change" — it comes through as undefined.
Details:
- The callback form of watch gets called with (values, { name, type }). The docs show that signature. (react-hook-form.com)
- In practice user input produces type === 'change', while calling setValue programmatically will trigger the watcher but the type field is undefined (not 'change'). This is reported in the project issues/threads. (github.com)
Notes / gotchas:
- useWatch/watch reactivity can be order-sensitive: if you set a value before a useWatch subscription is set up, that update may be missed. If you need to distinguish programmatic vs user changes, either check for type === 'change' (user) vs undefined (programmatic), or explicitly call a handler/emit your own event after calling setValue. (react-hook-form.com)
If you want, I can show a minimal example demonstrating watch receiving type 'change' for user input and undefined for setValue.
Citations:
- 1: https://react-hook-form.com/docs/useform/watch?utm_source=chatgpt.com
- 2: https://github.com/react-hook-form/documentation/issues/1040?utm_source=chatgpt.com
- 3: https://react-hook-form.com/docs/usewatch?utm_source=chatgpt.com
Apply source defaults on programmatic source changes
Programmatic calls to setValue('source', …) in the connection effect (packages/app/src/DBSearchPage.tsx lines 1241–1260) do invoke your watch subscription, but the { type } parameter is undefined—not 'change'. As a result, the reset logic in your watch handler (lines 842–869) never runs for programmatic source updates, leaving select, orderBy, and filters stale.
Two ways to address this:
• Extract a helper (e.g. applySourceDefaults) that:
- calls
setLastSelectedSourceId - sets
select&orderBydefaults - clears filters via
searchFilters.clearAllFilters() - triggers
debouncedSubmit()
and invoke it both:
– in your watch handler underif (name==='source' && type==='change')
– in the connection effect immediately aftersetValue('source', …)
• Or inline the same reset logic in the connection effect. For example:
// packages/app/src/DBSearchPage.tsx:1249–1257
if (!currentSource || currentSource.connection !== selectedConnectionId) {
setValue('source', firstMatching.id, {
shouldDirty: true,
shouldTouch: true,
});
+ // Apply source defaults for the new source
+ const newSource = inputSourceObjs.find(s => s.id === firstMatching.id);
+ if (newSource) {
+ setLastSelectedSourceId(newSource.id);
+ setValue('select', newSource.defaultTableSelectExpression ?? '');
+ setValue(
+ 'orderBy',
+ `${getFirstTimestampValueExpression(newSource.timestampValueExpression ?? '')} DESC`
+ );
+ searchFilters.clearAllFilters();
+ setTimeout(() => debouncedSubmit(), 0);
+ }
}Don’t forget to add the missing dependencies to that effect:
setLastSelectedSourceId, searchFilters, and debouncedSubmit.
📝 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.
| useEffect(() => { | |
| if (!selectedConnectionId || !inputSourceObjs) return; | |
| const allowedKinds = [SourceKind.Log, SourceKind.Trace]; | |
| const firstMatching = inputSourceObjs.find( | |
| s => | |
| s.connection === selectedConnectionId && allowedKinds.includes(s.kind), | |
| ); | |
| if (!firstMatching) return; | |
| const currentSourceId = watch('source'); | |
| const currentSource = inputSourceObjs.find( | |
| s => s.id === (currentSourceId as any), | |
| ); | |
| if (!currentSource || currentSource.connection !== selectedConnectionId) { | |
| setValue('source', firstMatching.id, { | |
| shouldDirty: true, | |
| shouldTouch: true, | |
| }); | |
| } | |
| }, [selectedConnectionId, inputSourceObjs, setValue, watch]); | |
| useEffect(() => { | |
| if (!selectedConnectionId || !inputSourceObjs) return; | |
| const allowedKinds = [SourceKind.Log, SourceKind.Trace]; | |
| const firstMatching = inputSourceObjs.find( | |
| s => | |
| s.connection === selectedConnectionId && allowedKinds.includes(s.kind), | |
| ); | |
| if (!firstMatching) return; | |
| const currentSourceId = watch('source'); | |
| const currentSource = inputSourceObjs.find( | |
| s => s.id === (currentSourceId as any), | |
| ); | |
| if (!currentSource || currentSource.connection !== selectedConnectionId) { | |
| setValue('source', firstMatching.id, { | |
| shouldDirty: true, | |
| shouldTouch: true, | |
| }); | |
| // Apply source defaults for the new source | |
| const newSource = inputSourceObjs.find(s => s.id === firstMatching.id); | |
| if (newSource) { | |
| setLastSelectedSourceId(newSource.id); | |
| setValue('select', newSource.defaultTableSelectExpression ?? ''); | |
| setValue( | |
| 'orderBy', | |
| `${getFirstTimestampValueExpression(newSource.timestampValueExpression ?? '')} DESC` | |
| ); | |
| searchFilters.clearAllFilters(); | |
| setTimeout(() => debouncedSubmit(), 0); | |
| } | |
| } | |
| }, [ | |
| selectedConnectionId, | |
| inputSourceObjs, | |
| setValue, | |
| watch, | |
| setLastSelectedSourceId, | |
| searchFilters, | |
| debouncedSubmit, | |
| ]); |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/DBSearchPage.tsx (2)
1106-1116: Initial search sends an ISO string to onSearch; parseTimeQuery likely failsonSearch expects a time query string parseable into [start, end] (e.g., via dateRangeToString or “Live Tail”), but here it receives a single ISO string, which will not round-trip through parseTimeQuery. This prevents the initial time range from being set.
Apply this diff to pass a proper formatted range and sync the input:
- if (!searchParams.has('from') && !searchParams.has('to')) { - const newTimeRange = new Date(defaultTimeRange[0]); - onSearch(newTimeRange.toISOString()); - } + if (!searchParams.has('from') && !searchParams.has('to')) { + const initial = dateRangeToString(defaultTimeRange, false); + setDisplayedTimeInputValue(initial); + onSearch(initial); + }
700-709: Conflicting default for isLive re-enables live tail when URL has no time rangeThis effect turns live tail on whenever from/to are not in the URL, which contradicts the new fixed 15-minute default and the earlier effect that forces isLive to false at init. Remove or invert this to avoid flicker/race.
Two options, pick one:
- Remove the effect entirely (recommended):
- useEffect(() => { - if (_isLive == null && isReady) { - if (from == null && to == null) { - setIsLive(true); - } else { - setIsLive(false); - } - } - }, [_isLive, setIsLive, from, to, isReady]); + // default isLive is false when starting from a fixed time range; no-op here
- Or invert the default to false:
- if (from == null && to == null) { - setIsLive(true); - } else { - setIsLive(false); - } + // Keep live tail off by default unless explicitly enabled + setIsLive(false);
♻️ Duplicate comments (1)
packages/app/src/config.ts (1)
14-14: Provide a safe default for HDX_AUTH_EMAILS to avoid surprises downstreamEven though the hook guards against falsy values, defaulting here makes the contract explicit and prevents future direct JSON.parse sites from crashing.
-export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS'); +export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS') ?? '[]';
🧹 Nitpick comments (4)
packages/app/src/TeamPage.tsx (2)
341-377: De-duplicate ky error handling across mutationsonSuccess/onError handlers are repeated with nearly identical HTTPError parsing. Extract a tiny helper to improve maintainability and consistency.
Example helper (module-level or a small util):
function showKyError(e: unknown, fallback = 'Something went wrong. Please contact HyperDX team.') { if (e instanceof HTTPError) { e.response.json() .then((res: any) => notifications.show({ color: 'red', message: res?.message ?? fallback, autoClose: 5000 })) .catch(() => notifications.show({ color: 'red', message: fallback, autoClose: 5000 })); } else { notifications.show({ color: 'red', message: fallback, autoClose: 5000 }); } }Then replace onError blocks with:
onError: (e) => showKyError(e),Also applies to: 400-435, 444-477
1132-1146: Prefer typing the error parameter instead of using an untyped callbackMinor nit: give onError an explicit parameter type to aid tooling and future refactors.
- onError: () => { + onError: (_e: unknown) => { notifications.show({ color: 'red', message: `Failed to update ${label}`, }); },packages/app/src/DBSearchPage.tsx (2)
1220-1226: Prefer using hasAccess(email) instead of indexing authArray directlyLeverage the hook’s API for clarity and guardrails. This avoids type gymnastics on authArray and centralizes logic (e.g., truthiness, defaulting).
- // Parse demo auth emails from environment variable - const { authArray } = useAuthEmails(); + // Parse demo auth emails from environment variable + const { hasAccess } = useAuthEmails();- {authArray[me?.email as keyof typeof authArray] && ( + {hasAccess(me?.email) && ( <Menu.Target> <ActionIcon variant="subtle" color="dark.2" size="sm" title="Edit Source" > <Text size="xs"> <i className="bi bi-gear" /> </Text> </ActionIcon> </Menu.Target> )}Also applies to: 1290-1304
1-1: Avoid disabling all ESLint rules at file scopePrefer targeted disables or fixing the underlying issues to keep lint coverage effective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/app/src/DBSearchPage.tsx(23 hunks)packages/app/src/TeamPage.tsx(15 hunks)packages/app/src/components/SourceSelect.tsx(3 hunks)packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useAuthEmails.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app/src/components/SourceSelect.tsx
- packages/app/src/hooks/useAuthEmails.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/app/src/TeamPage.tsx (1)
packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)
packages/app/src/DBSearchPage.tsx (5)
packages/app/src/connection.ts (1)
useConnections(43-54)packages/app/src/source.ts (1)
useSources(77-88)packages/app/src/timeQuery.ts (2)
dateRangeToString(38-48)useNewTimeQuery(432-530)packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)packages/app/src/components/SourceSelect.tsx (1)
SourceSelectControlled(67-67)
🔇 Additional comments (10)
packages/app/src/TeamPage.tsx (5)
43-43: Importing useAuthEmails to gate UI: LGTMThis aligns with the new access-control approach and keeps concerns separated in a hook.
298-305: Gating destructive actions behind hasAccess is correctConditionally showing “Remove” only when not current user, admin, and authorized is sound.
Also applies to: 334-334, 553-572
1053-1064: Team name “Change” button gating is consistent with auth modelConditionally showing the control only for admins with access aligns with the broader UI policy.
1444-1452: Personal API Key visibility gated by me != null: LGTMShowing the personal key card only when me is present prevents undefined access errors and is a reasonable UX.
Also applies to: 1349-1349
1460-1463: Section-level gating with hasAccess is applied consistentlySources, Connections, TeamQueryConfig, and API Keys are correctly gated. Integrations and Team Name remain open, which seems intentional.
Also applies to: 1483-1489
packages/app/src/DBSearchPage.tsx (5)
395-402: Fixed 15-minute range helper: LGTMThe helper is clear and the intent to avoid auto-updating is preserved via module-level initialization.
573-575: Time initialization flow matches the new default behavior
- Defaulting isLive to false is consistent.
- Initial display value and useNewTimeQuery wiring look correct.
Also applies to: 675-688, 690-696
119-120: Addingconnectionto schema and queryStateMap: LGTMOptional connection in the schema and query state will help persist the selection across reloads.
Also applies to: 507-508
600-637: Connection derivation and sync to URL are sensiblegetInitialConnection priority (URL → current source → last source → first connection) is reasonable, and the watch handler keeps URL state in sync.
Also applies to: 639-658, 870-892
1240-1260: Auto-align source to selected connection: LGTMEnsuring the selected source matches the active connection prevents cross-connection queries. The allowedKinds filter is a pragmatic constraint for current UI.
| sourceType={inputSourceObj?.kind} | ||
| showDelta={!!searchedSource?.durationExpression} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Passing SourceKind to filters matches the intent; verify the prop typing in the filters component
Good to pass inputSourceObj?.kind. Ensure DBSearchPageFilters (and any helper) now types sourceType?: SourceKind.
Run this to verify prop typing and usage:
🏁 Script executed:
#!/bin/bash
# Confirm DBSearchPageFilters prop and helper types accept SourceKind
echo "Searching for DBSearchPageFilters prop definitions..."
rg -n -C2 --type=ts --type=tsx 'interface\s+DBSearchPageFiltersProps|type\s+DBSearchPageFiltersProps'
echo "Verifying sourceType typing to SourceKind..."
rg -n -C2 --type=ts --type=tsx 'sourceType\??:\s*SourceKind'
echo "Checking imports of SourceKind in the filters file..."
fd DBSearchPageFilters.tsx | xargs -I{} rg -n -C2 'SourceKind' {}Length of output: 625
Let’s locate the filters file and search without relying on --type filters:
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Locate DBSearchPageFilters.tsx..."
FILTER_FILE=$(fd DBSearchPageFilters.tsx | head -n1)
echo " → $FILTER_FILE"
echo "2. Search for props definition (interface or type)..."
rg -n -C2 'DBSearchPageFiltersProps' "$FILTER_FILE"
echo "3. Check sourceType prop typing..."
rg -n -C2 'sourceType\??\s*:' "$FILTER_FILE"
echo "4. Verify import of SourceKind..."
rg -n -C2 'import.*SourceKind' "$FILTER_FILE"Length of output: 528
Let’s inspect the filters component to confirm its prop typing and imports:
🏁 Script executed:
#!/bin/bash
set -e
# Locate the Filters component file
FILTER_FILE=$(fd DBSearchPageFilters.tsx | head -n1)
echo "Using filter file: $FILTER_FILE"
# 1) Look for Props definition (interface or type)
echo -e "\n-- Props interface/type definitions --"
rg -n 'interface DBSearchPageFiltersProps|type DBSearchPageFiltersProps' "$FILTER_FILE"
# 2) Locate the component signature
echo -e "\n-- Component declaration --"
rg -n -C2 'DBSearchPageFilters' "$FILTER_FILE"
# 3) Find all sourceType occurrences in props or default values
echo -e "\n-- sourceType usages --"
rg -n -C2 'sourceType' "$FILTER_FILE"
# 4) Verify import of SourceKind
echo -e "\n-- SourceKind imports --"
rg -n -C2 'import.*SourceKind' "$FILTER_FILE"Length of output: 487
Let’s inspect the beginning of the filters component to see how props (including sourceType) are declared:
🏁 Script executed:
#!/bin/bash
set -e
FILE=packages/app/src/components/DBSearchPageFilters.tsx
echo "===== Displaying first 200 lines of $FILE ====="
sed -n '1,200p' "$FILE"Length of output: 5371
Let’s locate the actual component declaration and its props type further down in the file:
🏁 Script executed:
#!/bin/bash
set -e
FILE=packages/app/src/components/DBSearchPageFilters.tsx
echo "1) Search for the component export:"
rg -n 'export\s+(const|function)\s+DBSearchPageFilters' "$FILE" || echo "→ Not found via export pattern"
echo -e "\n2) Search all occurrences of DBSearchPageFilters:"
rg -n -C2 'DBSearchPageFilters' "$FILE"
echo -e "\n3) If we find the props usage, show 10 lines around it:"
# Replace <LINE> with the line number from step 2 if found; otherwise we'll show lines 200–400
rg -n -C5 'DBSearchPageFilters\(' "$FILE" || sed -n '200,400p' "$FILE"Length of output: 7632
Ensure DBSearchPageFilters accepts sourceType and showDelta
The filters component currently doesn’t declare sourceType or showDelta in its props, so passing them from DBSearchPage will break typing. Please update:
• packages/app/src/components/DBSearchPageFilters.tsx
– Define or extend a DBSearchPageFiltersProps interface (or type) that includes:
• sourceType?: SourceKind
• showDelta?: boolean
– Annotate the DBSearchPageFiltersComponent parameter with this props type.
– Update the helper signature:
```ts
// before
const getKeysForSourceType = (sourceType?: string) => { … }
// after
const getKeysForSourceType = (sourceType?: SourceKind) => { … }
```
– Add an import for SourceKind at the top of the file.
🤖 Prompt for AI Agents
In packages/app/src/components/DBSearchPageFilters.tsx around the top and where
the component is declared, add or extend a DBSearchPageFiltersProps type that
includes sourceType?: SourceKind and showDelta?: boolean, import SourceKind from
its module at the top of the file, annotate the DBSearchPageFiltersComponent
function parameter with this DBSearchPageFiltersProps type, and change the
helper signature from getKeysForSourceType = (sourceType?: string) => { … } to
getKeysForSourceType = (sourceType?: SourceKind) => { … } so the component
accepts the props passed from DBSearchPage.tsx.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
packages/app/src/components/DBSearchPageFilters.tsx (6)
233-237: Gate “Load more” auto-trigger to avoid N extra network calls on mountAuto-triggering onLoadMore for every facet is costly. Gate by a “pinned field” signal and include it in deps.
-useEffect(() => { - if (onLoadMore && !hasLoadedMore) { - onLoadMore(name); - } -}, [onLoadMore, hasLoadedMore, name]); +useEffect(() => { + if (onLoadMore && !hasLoadedMore && isFieldPinned) { + onLoadMore(name); + } +}, [onLoadMore, hasLoadedMore, name, isFieldPinned]);
448-466: Remove unused prop sourceType (noise in deps and logs)sourceType is not used for behavior. Drop it from props and all references to reduce noise and accidental re-renders.
sourceId, - sourceType, showDelta, denoiseResults, setDenoiseResults, sourceName, connectionName, }: { @@ chartConfig: ChartConfigWithDateRange; sourceId?: string; - sourceType?: string; showDelta: boolean; denoiseResults: boolean; setDenoiseResults: (denoiseResults: boolean) => void; sourceName?: string; connectionName?: string;Note: also remove sourceType from any dependency arrays and debug logs below.
496-496: Remove unused useExplainQuery (saves an extra ClickHouse round-trip)countData is only referenced in removed debug logs. Drop the hook call (and the import at top).
- const { data: countData } = useExplainQuery(chartConfig);Also remove the import:
- import { useExplainQuery } from '@/hooks/useExplainQuery';
562-565: Heads-up: limit: keyLimit won’t be honored until hook bug is fixeduseGetKeyValues currently hardcodes limit: 1000 internally, ignoring the passed limit. Please patch packages/app/src/hooks/useMetadata.tsx accordingly.
Apply in useMetadata.tsx (outside this diff):
- limit: 1000, + limit,Verification script:
#!/bin/bash rg -nP '\blimit\s*:\s*1000\b' packages/app/src/hooks/useMetadata.tsx -n -C2
603-604: Fix callback dependencies to avoid stale facets configloadMoreFilterValuesForKey closes over facetsChartConfig and dateRange; remove unused deps and include facetsChartConfig.
- [chartConfig, setExtraFacets, dateRange, sourceType], + [facetsChartConfig, dateRange],
467-514: Strip debug console logging (risk of PII + noisy prod); use dev-only guard if neededLarge swaths of console.error logging in the render path will leak identifiers and bloat logs. Remove them or gate behind a dev-only guard; also avoid console in catch.
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); @@ - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); @@ - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); @@ - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); @@ - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error('');Prefer a dev-only pattern in the catch:
- } catch (error) { - console.error('failed to fetch more keys', error); + } catch (error) { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn('failed to fetch more keys', error); + }Also applies to: 493-495, 504-513, 509-514, 594-596
🧹 Nitpick comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
32-63: Align override map typing and intent (currently untyped; joined-key path can’t match this map)serviceMapOverride only has generic type keys (Logs/Traces/K8sEvents). The joined "connectionName | sourceName" lookup in getKeysForSourceType cannot hit this map, making that primary branch dead. Also add an explicit type to prevent accidental value shape drift.
Apply:
-// Override keys for specific source types -const serviceMapOverride = { +// Override keys for specific source types (and optionally connection|source tuples) +const serviceMapOverride: Record<string, string[]> = {
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (5)
packages/common-utils/src/types.ts (1)
ChartConfigWithDateRange(419-419)packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (2)
useAllFields(82-126)useGetKeyValues(156-207)packages/app/src/utils.ts (1)
mergePath(649-655)
🔇 Additional comments (3)
packages/app/src/components/DBSearchPageFilters.tsx (3)
122-124: LGTM: stable noop avoids double-firingemptyFn keeps Checkbox onChange from double-firing because Group handles toggling; good pattern.
549-556: LGTM: facetsChartConfig correctly ignores current filtersDropping where/orderBy for facet discovery avoids narrowing option sets; good call.
609-646: LGTM: facet merging and pin-first orderingMerging extraFacets and reordering by pinned fields looks correct and stable.
| const fieldKeys = useMemo(() => { | ||
| const fields = data ?? []; | ||
| // Map Field.path to SQL-style key notation used in filters (e.g., ResourceAttributes['k8s.namespace.name']) | ||
| const keys = new Set<string>(); | ||
| for (const f of fields as any[]) { | ||
| if (f?.path && Array.isArray(f.path)) { | ||
| const key = mergePath(f.path); | ||
| if (key) keys.add(key); | ||
| } | ||
| } | ||
| return Array.from(keys); | ||
| }, [data]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use metadata-derived fieldKeys as a fallback when overrides are empty
When overrides don’t match, keysToFetch is [], useGetKeyValues is disabled, and users see “No filters available” despite metadata. Fall back to fieldKeys.
-const keysToFetch = useMemo(() => {
- const overrides = getKeysForSourceType(sourceName, connectionName);
- return overrides;
-}, [sourceName, connectionName]);
+const keysToFetch = useMemo(() => {
+ const overrides = getKeysForSourceType(sourceName, connectionName);
+ return overrides.length ? overrides : fieldKeys;
+}, [sourceName, connectionName, fieldKeys]);Also applies to: 528-532
🤖 Prompt for AI Agents
In packages/app/src/components/DBSearchPageFilters.tsx around lines 515-526 (and
similarly lines 528-532), the current logic builds fieldKeys from metadata but
later paths use overrides/keysToFetch and disable useGetKeyValues which can
leave keysToFetch empty and show “No filters available”; update the conditional
flow so that when overrides or keysToFetch are empty (or useGetKeyValues is
false) you fall back to using the computed fieldKeys: if keysToFetch is empty,
populate it from fieldKeys before deciding to disable filters or skip fetching
values, and ensure the same fallback is applied in the other block at 528-532 so
metadata-derived keys are used whenever overrides are absent.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/DBSearchPage.tsx (2)
474-486: Bug: tableFilterExpression is dropped when user filters exist (filters overridden)The current construction sets filters twice; when filters != null, it overrides the earlier array that included tableFilterExpression, losing the implicit table constraint. Build filters once, combining both inputs.
Apply:
- ...(sourceObj.tableFilterExpression != null - ? { - filters: [ - { - type: 'sql' as const, - condition: sourceObj.tableFilterExpression, - }, - ...(filters ?? []), - ], - } - : {}), - ...(filters != null ? { filters } : {}), + ...(sourceObj.tableFilterExpression != null || filters != null + ? { + filters: [ + ...(sourceObj.tableFilterExpression + ? [ + { + type: 'sql' as const, + condition: sourceObj.tableFilterExpression, + }, + ] + : []), + ...(filters ?? []), + ], + } + : {}),
1066-1076: Initial search uses an invalid timeQuery string (only start ISO); call onTimeRangeSelect insteadonSearch expects a parseable timeQuery string (e.g., “start - end”), but this effect passes only an ISO start. Use onTimeRangeSelect with both dates to set from/to correctly.
- if (!searchParams.has('from') && !searchParams.has('to')) { - const newTimeRange = new Date(defaultTimeRange[0]); - onSearch(newTimeRange.toISOString()); - } + if (!searchParams.has('from') && !searchParams.has('to')) { + const [start, end] = defaultTimeRange; + onTimeRangeSelect(start, end); + } - }, [isReady, queryReady, isChartConfigLoading, onSearch]); + }, [isReady, queryReady, isChartConfigLoading, onTimeRangeSelect]);
♻️ Duplicate comments (4)
packages/app/src/DBSearchPage.tsx (4)
462-503: Add connectionName to memo deps to avoid stale labels in charts/filtersconnectionName is derived via useConnections and included in the returned config, but it’s not in the useMemo dependency array. This can leave a stale connectionName in downstream UIs when connections resolve or change.
Apply this patch:
return useMemo(() => { if (sourceObj != null) { return { data: { select: select || (sourceObj.defaultTableSelectExpression ?? ''), from: sourceObj.from, - ...(sourceObj.tableFilterExpression != null - ? { - filters: [ - { - type: 'sql' as const, - condition: sourceObj.tableFilterExpression, - }, - ...(filters ?? []), - ], - } - : {}), - ...(filters != null ? { filters } : {}), + // See next comment for a safer filters construction where: where ?? '', whereLanguage: whereLanguage ?? 'sql', timestampValueExpression: sourceObj.timestampValueExpression, implicitColumnExpression: sourceObj.implicitColumnExpression, connection: sourceObj.connection, connectionName, displayType: DisplayType.Search, orderBy: orderBy || `${getFirstTimestampValueExpression( sourceObj.timestampValueExpression, )} DESC`, }, }; } return { data: null, isLoading }; - }, [sourceObj, isLoading, select, filters, where, whereLanguage, orderBy]); + }, [sourceObj, isLoading, select, filters, where, whereLanguage, orderBy, connectionName]);
819-857: Watchers: user-driven updates covered; ensure programmatic updates apply defaults too
- Source watcher (type === 'change') resets defaults and clears filters — good.
- Connection watcher (type === 'change') updates URL and refreshes — good.
But programmatic setValue does not set type === 'change'. When you call setValue('source', ...) in effects, the watcher won’t reset select/orderBy/filters. See fix below in the connection→source effect.
1546-1549: Filters component receives new props; ensure its typings include themDBSearchPageFilters now receives:
- sourceType?: SourceKind
- sourceName?: string
- connectionName?: string
Please verify DBSearchPageFilters’ props/types are updated accordingly (and that SourceKind is imported in that file). Prior feedback covered sourceType/showDelta; connectionName and sourceName need the same treatment.
#!/bin/bash # Verify DBSearchPageFilters props include the new fields and SourceKind FILE=$(fd DBSearchPageFilters.tsx | head -n1) echo "Inspecting $FILE" rg -n -C2 'interface\s+DBSearchPageFiltersProps|type\s+DBSearchPageFiltersProps' "$FILE" rg -n -C2 'sourceType\??:\s*SourceKind' "$FILE" rg -n -C2 'sourceName\??:\s*string' "$FILE" rg -n -C2 'connectionName\??:\s*string' "$FILE" rg -n -C2 'import\s+{\s*SourceKind\s*}' "$FILE"
1201-1227: Programmatic source selection doesn’t apply defaults/filters resetWhen the connection effect sets a new source (setValue('source', …)), the watch(handler) for source changes won’t run because type !== 'change'. Inline the same defaults/reset logic here.
if (!currentSource) { setValue('source', firstMatching.id, { shouldDirty: true, shouldTouch: true, }); - return; + // Apply the same defaults as the user-driven source change + setLastSelectedSourceId(firstMatching.id); + setValue('select', firstMatching.defaultTableSelectExpression ?? ''); + setValue( + 'orderBy', + `${getFirstTimestampValueExpression(firstMatching.timestampValueExpression ?? '')} DESC`, + ); + searchFilters.clearAllFilters(); + setTimeout(() => debouncedSubmit(), 0); + return; } // If there is a mismatch, prefer keeping the current source and align the connection to it if (currentSource.connection !== selectedConnectionId) { setValue('connection', currentSource.connection as any); setSearchedConfig(prev => ({ ...prev, connection: currentSource.connection })); } -}, [selectedConnectionId, inputSourceObjs, setValue, setSearchedConfig, watch]); +}, [selectedConnectionId, inputSourceObjs, setValue, setSearchedConfig, watch, setLastSelectedSourceId, searchFilters, debouncedSubmit]);
🧹 Nitpick comments (7)
packages/app/src/DBSearchPage.tsx (7)
1-1: Avoid file-wide ESLint disable; scope or justify rule exceptionsA blanket /* eslint-disable / can hide legitimate issues. Prefer targeted disables (/ eslint-disable-next-line */) at the smallest scope or document which rules are intentionally disabled and why.
119-134: Schema extended with optional connection; ensure it’s persisted and shareableYou added connection?: string to the zod schema, but:
- Save/Update flows in SaveSearchModal do not persist connection.
- generateSearchUrl omits connection, so shared URLs may lose context.
Consider persisting and serializing connection too. See separate comment on generateSearchUrl for a concrete patch.
395-402: Fixed default time window: OK, but compute at mount for accuracycreateFixedTimeRange is good for a non-live default. Minor: defining defaultTimeRange at module init may capture time slightly earlier than first render. If you want “15 minutes before first render,” compute it in a useMemo inside the component.
656-678: Initial time display may ignore user UTC preferenceinitialTimeDisplay is built with dateRangeToString(defaultTimeRange, false) and then passed to useNewTimeQuery as initialDisplayValue. This may disagree with a user’s isUTC preference that useNewTimeQuery applies elsewhere.
Options:
- Pass undefined for initialDisplayValue and let useNewTimeQuery derive it from initialTimeRange + isUTC.
- Or compute initialTimeDisplay with the user preference.
Example (preferred minimal change):
- const initialTimeDisplay = useMemo(() => { - return dateRangeToString(defaultTimeRange, false); - }, []); + const initialTimeDisplay = undefined; // let useNewTimeQuery format using user isUTCAnd update the useNewTimeQuery call to handle undefined initialDisplayValue (it already does).
1028-1045: Include connection in shareable search URLConnection is part of state now but isn’t serialized into generateSearchUrl. Add it so links retain the connection context.
const qParams = new URLSearchParams({ where: where || searchedConfig.where || '', whereLanguage: whereLanguage || 'sql', from: searchedTimeRange[0].getTime().toString(), to: searchedTimeRange[1].getTime().toString(), select: searchedConfig.select || '', source: searchedSource?.id || '', filters: JSON.stringify(searchedConfig.filters), }); + if (searchedConfig.connection) { + qParams.set('connection', searchedConfig.connection); + } return `/search?${qParams.toString()}`;
1184-1199: When auto-setting connection, also trigger a refreshThe effect sets form state and URL with the derived connection but doesn’t submit. If the intended UX is “auto-refresh on connection availability,” add a debounced submit.
if (!currentConnection && inputSourceObjs && connections && connections.length > 0) { const preferredConnectionId = getInitialConnection(); if (preferredConnectionId) { setValue('connection', preferredConnectionId); // Update URL state as well setSearchedConfig(prev => ({ ...prev, connection: preferredConnectionId, })); + // Trigger refresh for the resolved connection + setTimeout(() => debouncedSubmit(), 0); } } -}, [connections, inputSourceObjs, setValue, watch, getInitialConnection, setSearchedConfig]); +}, [connections, inputSourceObjs, setValue, watch, getInitialConnection, setSearchedConfig, debouncedSubmit]);
1548-1549: Avoid (filtersChartConfig as any)?.connectionName; carry meta separately or type itTo prevent any-casts, either:
- Extend the chart config type to include a optional meta: { connectionName?: string }, or
- Compute connectionName in this component (via connections + searchedSource.connection) and pass it directly.
Small but improves type safety.
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/DBSearchPage.tsx(27 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/DBSearchPage.tsx (5)
packages/app/src/connection.ts (1)
useConnections(43-54)packages/app/src/source.ts (1)
useSources(77-88)packages/app/src/timeQuery.ts (2)
dateRangeToString(38-48)useNewTimeQuery(432-530)packages/app/src/hooks/useAuthEmails.tsx (1)
useAuthEmails(9-48)packages/app/src/components/SourceSelect.tsx (1)
SourceSelectControlled(70-70)
🔇 Additional comments (8)
packages/app/src/DBSearchPage.tsx (8)
82-108: New imports wiring looks consistent; verify side-effects and caching
- useAuthEmails and useAliasMapFromChartConfig are used later and look fine.
- useConnections import is appropriate; React Query caching should dedupe the duplicate useConnections call in this file (one inside useSearchedConfigToChartConfig and one in the page), so no concern.
514-515: Connection added to URL state: LGTMqueryStateMap now includes connection, enabling URL/state sync. This aligns with the new schema and form value.
556-687: Live Tail default/off ramp looks consistent
- isLive explicitly defaults to false (Line 556), and several interactions disable it intentionally (Lines 565–574, 559–563, 665–669, 683–686). This matches the “fixed window” default UX.
702-709: URL→form sync preserves/derives connection: LGTMThe reset merges searchedConfig.connection or preserves existing form connection, otherwise falls back via getInitialConnection(). Good guardrails for history navigation.
997-1012: Derived table config memo: OKdbSqlRowTableConfig memoization over chartConfig and searchedTimeRange is appropriate.
1243-1246: Connection selector commented out — confirm this UXIf hiding connection selection is intentional (deriving it from source), great. Otherwise, consider keeping it available for advanced users. Note: your watchers and effects already handle connection changes correctly.
1258-1271: Auth-gated “Edit Source” gear: LGTMConditional rendering based on useAuthEmails and me.email is straightforward and safe; the button is entirely hidden for unauthorized users.
1472-1477: TimePicker live toggle off: LGTMshowLive={false} and forcing setIsLive(false) on search aligns with the fixed-window UX.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/common-utils/src/metadata.ts (1)
491-496: Removeread_overflow_mode: 'break'from all metadata queries and update tests
- In
packages/common-utils/src/metadata.ts, removeread_overflow_mode: 'break'and drop theString(...)wrapper aroundDEFAULT_METADATA_MAX_ROWS_TO_READin every occurrence (three spots where you guard ondisableRowLimit).- Update
packages/common-utils/src/__tests__/metadata.test.tsto no longer expectread_overflow_modein the generated settings.- Review
packages/common-utils/src/clickhouse.ts’s defaultread_overflow_mode: 'break'—switch to server defaults or'throw'if you want errors on overflow rather than silent truncation.
♻️ Duplicate comments (9)
packages/common-utils/src/clickhouse.ts (1)
466-471: Do not set read_overflow_mode='break' globally; this risks silent, truncated resultsSetting read_overflow_mode='break' at a global/default level can silently truncate datasets when max_rows_to_read hits, corrupting charts/metrics. Let the server default (usually 'throw') apply or allow callers to opt in per query. Also switch the row limit to a numeric literal for clarity.
Apply this diff:
- // Apply global defaults for all queries - clickhouse_settings = { - max_rows_to_read: '10000000', - read_overflow_mode: 'break', - ...clickhouse_settings, - } as ClickHouseSettings; + // Apply safe defaults for all queries (callers can override) + clickhouse_settings = { + max_rows_to_read: 10_000_000, + // Intentionally omit read_overflow_mode to avoid silent truncation; callers can override per-query. + ...clickhouse_settings, + } as ClickHouseSettings;Run to find any remaining occurrences forcing 'break':
#!/bin/bash rg -nP -C2 "read_overflow_mode\s*:\s*['\"]break['\"]"packages/app/src/components/DBSearchPageFilters.tsx (8)
256-261: Avoid eager “Load more” for every facet on mountThis causes N extra metadata calls. Gate by field pin.
-useEffect(() => { - if (onLoadMore && !hasLoadedMore) { - onLoadMore(name); - } -}, [onLoadMore, hasLoadedMore, name]); +useEffect(() => { + if (onLoadMore && !hasLoadedMore && isFieldPinned) { + onLoadMore(name); + } +}, [onLoadMore, hasLoadedMore, name, isFieldPinned]);
22-22: Drop useExplainQuery; it’s not used functionally and adds a queryRemove import and hook call.
-import { useExplainQuery } from '@/hooks/useExplainQuery';- const { data: countData } = useExplainQuery(chartConfig);Also applies to: 519-519
624-625: Fix callback dependencies (avoid stale facets config; drop unused deps)Use facetsChartConfig and dateRange; remove chartConfig, sourceType, and the setter.
- [chartConfig, setExtraFacets, dateRange, sourceType], + [facetsChartConfig, dateRange],
471-471: sourceType appears unused; remove or wire it into logicIt’s only in logs/deps; safe to drop if call sites don’t rely on it.
Run to verify before removal:
#!/bin/bash # Find prop usage and references rg -nP '\bsourceType\b' -C2If unused, remove from props/types and from the loadMore callback deps:
- sourceType, @@ - sourceType?: string;Also applies to: 483-483
102-109: Harden getKeysForSourceType: trim/guard + explicit return typePrevents undefined/whitespace joins and makes behavior predictable.
-const getKeysForSourceType = (sourceName?: string, connectionName?: string) => { - const newSourceKey = connectionName + ' | ' + sourceName; - if (newSourceKey && newSourceKey in serviceMapOverride) { - return serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride]; - } - return []; -}; +const getKeysForSourceType = ( + sourceName?: string, + connectionName?: string, +): string[] => { + const src = sourceName?.trim(); + const conn = connectionName?.trim(); + if (!src || !conn) return []; + const newSourceKey = `${conn} | ${src}`; + if (newSourceKey in serviceMapOverride) { + return serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride]; + } + return []; +};
490-518: Remove production console logging (noisy + PII risk)Large debug dumps should not ship.
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); + // (debug logging removed) @@ - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); + // (debug logging removed) @@ - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); + // (debug logging removed) @@ - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); const [showMoreFields, setShowMoreFields] = useState(false); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error('');Also applies to: 527-537
538-555: Use metadata-derived fieldKeys as fallback when no override matchesPrevents “No filters available” when overrides are absent.
const fieldKeys = useMemo(() => { const fields = data ?? []; @@ return Array.from(keys); }, [data]); -const keysToFetch = useMemo(() => { - const overrides = getKeysForSourceType(sourceName, connectionName); - return overrides; -}, [sourceName, connectionName]); +const keysToFetch = useMemo(() => { + const overrides = getKeysForSourceType(sourceName, connectionName); + return overrides.length ? overrides : fieldKeys; +}, [sourceName, connectionName, fieldKeys]);
583-586: Hook ignores caller limit; update useGetKeyValues to honor itYour limit: keyLimit (200) is ignored because the hook hardcodes 1000.
Apply in packages/app/src/hooks/useMetadata.tsx:
- limit: 1000, + limit,
🧹 Nitpick comments (1)
packages/common-utils/src/metadata.ts (1)
451-451: Limit bumped to 1000—confirm UX and latency at scaleA default of 1000 unique values per key can be heavy for wide keys. Ensure UI can handle it and dashboards remain responsive. Consider a defensive clamp or surfacing this via config.
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)packages/common-utils/src/clickhouse.ts(1 hunks)packages/common-utils/src/metadata.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/app/src/components/DBSearchPageFilters.tsx (5)
packages/common-utils/src/types.ts (1)
ChartConfigWithDateRange(419-419)packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (2)
useAllFields(82-126)useGetKeyValues(156-207)packages/app/src/utils.ts (1)
mergePath(649-655)
packages/common-utils/src/metadata.ts (1)
packages/common-utils/src/types.ts (1)
ChartConfigWithDateRange(419-419)
🪛 ESLint
packages/app/src/components/DBSearchPageFilters.tsx
[error] 77-77: Delete ···
(prettier/prettier)
[error] 92-92: Delete ···
(prettier/prettier)
🔇 Additional comments (1)
packages/common-utils/src/metadata.ts (1)
18-18: Raising DEFAULT_METADATA_MAX_ROWS_TO_READ to 10e6—validate cluster safetyThis can increase CPU/memory and query time notably. Confirm CH server/user-level limits (max_rows_to_read, max_result_rows, max_bytes_to_read, timeout) and dashboard latency budgets.
| 'US | Logs': [ | ||
| 'SeverityText ', | ||
| 'ServiceName', | ||
| "ResourceAttributes['k8s.cluster.name']", | ||
| "ResourceAttributes['k8s.namespace.name']", | ||
| "ResourceAttributes['k8s.container.name']", | ||
| "ResourceAttributes['label.team']", | ||
| "ResourceAttributes['label.pod']", | ||
| ], | ||
| 'US | Traces': [ |
There was a problem hiding this comment.
Fix override entries: trailing space and duplicate key break matching; clean up lint errors
- 'SeverityText ' has a trailing space; it won’t match the actual field.
- Duplicate "ResourceAttributes['label.pod']" in US | Traces.
- Trailing spaces (Prettier errors at Lines 77, 92).
Apply:
'US | Logs': [
- 'SeverityText ',
+ 'SeverityText',
'ServiceName',
@@
'US | Traces': [
@@
- "ResourceAttributes['label.pod']",
- "ResourceAttributes['label.pod']",
+ "ResourceAttributes['label.pod']",
@@
'Sgp | Logs': [
@@
- "ResourceAttributes['label.pod']",
+ "ResourceAttributes['label.pod']",Also applies to: 68-79, 85-93
🤖 Prompt for AI Agents
In packages/app/src/components/DBSearchPageFilters.tsx around lines 59-68 (and
also review lines 68-79 and 85-93), fix the override entries: remove the
trailing space from 'SeverityText ' so it exactly matches the actual field name,
remove the duplicate "ResourceAttributes['label.pod']" entry in the US | Traces
list, and trim any trailing spaces on the affected lines to resolve
Prettier/lint errors; ensure no duplicate keys remain and reformat the arrays so
each field string matches the real field name exactly.
| const cacheKey = [ | ||
| chartConfig.from.databaseName, | ||
| chartConfig.from.tableName, | ||
| keys.join(','), | ||
| chartConfig.dateRange.toString(), | ||
| String(disableRowLimit), | ||
| chartConfig.connection ?? '', | ||
| chartConfig.whereLanguage ?? '', | ||
| chartConfig.where ?? '', | ||
| JSON.stringify(chartConfig.filters ?? []), | ||
| ].join('.'); | ||
|
|
There was a problem hiding this comment.
Cache-key bug: dateRange.toString() yields “[object Object]” → collisions/stale data
Using Object.prototype.toString on dateRange produces “[object Object]”, so different ranges share the same cache entry. Serialize deterministically.
Apply this diff:
- const cacheKey = [
- chartConfig.from.databaseName,
- chartConfig.from.tableName,
- keys.join(','),
- chartConfig.dateRange.toString(),
- String(disableRowLimit),
- chartConfig.connection ?? '',
- chartConfig.whereLanguage ?? '',
- chartConfig.where ?? '',
- JSON.stringify(chartConfig.filters ?? []),
- ].join('.');
+ const cacheKey = JSON.stringify({
+ from: chartConfig.from,
+ keys,
+ dateRange: chartConfig.dateRange, // stable object serialization
+ disableRowLimit,
+ connection: chartConfig.connection ?? '',
+ whereLanguage: chartConfig.whereLanguage ?? '',
+ where: chartConfig.where ?? '',
+ filters: chartConfig.filters ?? [],
+ });📝 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.
| const cacheKey = [ | |
| chartConfig.from.databaseName, | |
| chartConfig.from.tableName, | |
| keys.join(','), | |
| chartConfig.dateRange.toString(), | |
| String(disableRowLimit), | |
| chartConfig.connection ?? '', | |
| chartConfig.whereLanguage ?? '', | |
| chartConfig.where ?? '', | |
| JSON.stringify(chartConfig.filters ?? []), | |
| ].join('.'); | |
| const cacheKey = JSON.stringify({ | |
| from: chartConfig.from, | |
| keys, | |
| dateRange: chartConfig.dateRange, // stable object serialization | |
| disableRowLimit, | |
| connection: chartConfig.connection ?? '', | |
| whereLanguage: chartConfig.whereLanguage ?? '', | |
| where: chartConfig.where ?? '', | |
| filters: chartConfig.filters ?? [], | |
| }); |
🤖 Prompt for AI Agents
In packages/common-utils/src/metadata.ts around lines 460 to 471, the cache key
uses chartConfig.dateRange.toString(), which yields "[object Object]" and causes
cache collisions; replace that with a deterministic serialization such as
JSON.stringify(chartConfig.dateRange ?? {}) (or JSON.stringify with sorted keys
if you need stable ordering) so different date ranges produce distinct cache
keys and handle undefined/null safely.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
22-22: Remove debug logging and unused explain hookNoisy console logging risks PII; useExplainQuery is only used for logs.
-import { useExplainQuery } from '@/hooks/useExplainQuery';- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); - const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = - usePinnedFilters(sourceId ?? null); - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); - const { width, startResize } = useResizable(16, 'left'); - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); - const { data: countData } = useExplainQuery(chartConfig); - const { data, isLoading } = useAllFields({ + const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = + usePinnedFilters(sourceId ?? null); + const { width, startResize } = useResizable(16, 'left'); + const { data, isLoading } = useAllFields({ databaseName: chartConfig.from.databaseName, tableName: chartConfig.from.tableName, connectionId: chartConfig.connection, }); - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); - const [showMoreFields, setShowMoreFields] = useState(false); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error(''); + const [showMoreFields, setShowMoreFields] = useState(false);Also applies to: 490-537, 519-519
♻️ Duplicate comments (6)
packages/app/src/components/DBSearchPageFilters.tsx (6)
256-261: Don’t auto “Load more” for every facet on mount (perf/cost)Gate by pinned fields (or remove auto-run).
-useEffect(() => { - if (onLoadMore && !hasLoadedMore) { - onLoadMore(name); - } -}, [onLoadMore, hasLoadedMore, name]); +useEffect(() => { + if (onLoadMore && !hasLoadedMore && isFieldPinned) { + onLoadMore(name); + } +}, [onLoadMore, hasLoadedMore, name, isFieldPinned]);
583-586: Minor: ensure hook honors the passed limit (external file change)If useGetKeyValues internally hardcodes limit (e.g., 1000), the 200 here is ignored. Please adjust the hook to forward the caller’s limit.
#!/bin/bash # Verify hook respects caller's limit rg -nP '\blimit\s*:\s*1000\b' packages/app/src/hooks/useMetadata.tsx -C2 || true rg -nP 'useGetKeyValues\(\s*\{' packages/app/src/components/DBSearchPageFilters.tsx -n -A5 -B2
59-61: Fix override typos/duplicates so matches actually work
- 'SeverityText ' has a trailing space → won’t match.
- Duplicate "ResourceAttributes['label.pod']" under US | Traces.
'US | Logs': [ - 'SeverityText ', + 'SeverityText', 'ServiceName', @@ 'US | Traces': [ 'ServiceName', 'StatusCode', 'SpanKind', "ResourceAttributes['k8s.cluster.name']", "ResourceAttributes['k8s.namespace.name']", "ResourceAttributes['k8s.container.name']", "ResourceAttributes['label.team']", - "ResourceAttributes['label.pod']", - "ResourceAttributes['label.pod']", + "ResourceAttributes['label.pod']", ],Also applies to: 68-79, 85-93
102-109: Harden getKeysForSourceType (guard/trim/explicit type)Avoid undefined joins and normalize inputs; add explicit return type.
-// Helper function to get keys - override for specific types, use default for others -const getKeysForSourceType = (sourceName?: string, connectionName?: string) => { - const newSourceKey = connectionName + ' | ' + sourceName; - if (newSourceKey && newSourceKey in serviceMapOverride) { - return serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride]; - } - return []; -}; +// Helper function to get keys - override for specific types, use default for others +const getKeysForSourceType = ( + sourceName?: string, + connectionName?: string, +): string[] => { + const src = sourceName?.trim(); + const conn = connectionName?.trim(); + if (!src || !conn) return []; + const newSourceKey = `${conn} | ${src}`; + return ( + serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride] ?? [] + ); +};
551-555: Use metadata-derived fieldKeys as fallback to avoid “No filters available”When no override matches, fetch by discovered fields.
-const keysToFetch = useMemo(() => { - const overrides = getKeysForSourceType(sourceName, connectionName); - return overrides; -}, [sourceName, connectionName]); +const keysToFetch = useMemo(() => { + const overrides = getKeysForSourceType(sourceName, connectionName); + return overrides.length ? overrides : fieldKeys; +}, [sourceName, connectionName, fieldKeys]);Also applies to: 538-549
624-624: Fix callback deps (avoid stale facets config; remove unused deps)Use facetsChartConfig/dateRange; drop chartConfig/setExtraFacets/sourceType.
- [chartConfig, setExtraFacets, dateRange, sourceType], + [facetsChartConfig, dateRange],
🧹 Nitpick comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
471-477: Drop unused sourceType prop if no longer readAfter removing debug logs there’s no usage. Safer to delete prop to avoid confusion; update call sites accordingly.
- sourceId, - sourceType, + sourceId, @@ - sourceType?: string; @@ - sourceName?: string; - connectionName?: string; + sourceName?: string; + connectionName?: string;Would you like a follow-up PR to update DBSearchPage.tsx and other call sites?
Also applies to: 483-489
615-615: Avoid console in production error pathUse a dev-only guard or app logger.
- console.error('failed to fetch more keys', error); + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.error('failed to fetch more keys', error); + }
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)
🔇 Additional comments (1)
packages/app/src/components/DBSearchPageFilters.tsx (1)
627-667: Facet merging + pinned-first ordering looks solidMerges extra values without dupes and prioritizes pinned fields.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
packages/app/src/config.ts (1)
14-17: Fix formatting issues and improve error handlingThe current implementation has formatting issues flagged by Prettier and lacks proper error handling for the Number conversion.
Apply this diff to fix both issues:
-export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS'); -export const HDX_METADATA_MAX_ROWS_TO_READ = Number( - env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000' -); +export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS') ?? '[]'; +export const HDX_METADATA_MAX_ROWS_TO_READ = Number( + env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000', +);This addresses:
- HDX_AUTH_EMAILS: Provides a safe default to prevent
JSON.parse(undefined)crashes downstream- HDX_METADATA_MAX_ROWS_TO_READ: Adds missing trailing comma for Prettier compliance
packages/app/src/hooks/useMetadata.tsx (1)
191-193: Review the hardcoded limits and their interaction with function parametersThere are a few concerns with these hardcoded values:
Keys limit increased: The change from 20 to 50 keys significantly increases the potential for request header size issues (mentioned in the comment). Has this been tested with larger key sets?
Limit parameter shadowing: The hardcoded
limit: 1000completely ignores the function'slimitparameter, which could confuse callers who expect their limit to be respected.Consider this approach to respect the caller's intent while providing sensible defaults:
-keys: keys.slice(0, 50), // Limit to 50 keys for now, otherwise request fails (max header size) -limit: 1000, +keys: keys.slice(0, Math.min(keys.length, 50)), // Limit to 50 keys for now, otherwise request fails (max header size) +limit: limit ?? 1000,Let me verify if these limits are used consistently across the codebase:
#!/bin/bash # Description: Check how useGetKeyValues limit parameter is used and if the hardcoded limits are appropriate # Search for useGetKeyValues usage patterns rg -n --type=tsx --type=ts "useGetKeyValues" -A 10 -B 5 # Look for other key/limit configurations rg -n --type=tsx --type=ts "keys.*slice.*\d+" -C 3 # Search for limit parameter usage patterns rg -n --type=tsx --type=ts "limit.*\d+" -C 3packages/app/src/components/DBSearchPageFilters.tsx (9)
267-271: Don’t auto “Load more” for every facet (cost/perf); gate by pinnedPrevents N extra metadata calls on mount.
Apply:
useEffect(() => { - if (onLoadMore && !hasLoadedMore) { + if (onLoadMore && !hasLoadedMore && isFieldPinned) { onLoadMore(name); } -}, [onLoadMore, hasLoadedMore, name]); +}, [onLoadMore, hasLoadedMore, name, isFieldPinned]);
22-22: Drop unused explain query (unnecessary network call)No longer used after removing debug logs.
Apply:
-import { useExplainQuery } from '@/hooks/useExplainQuery';- const { data: countData } = useExplainQuery(chartConfig);Also applies to: 530-530
611-616: Use the same limit constant and fix callback depsAvoid magic number; prevent stale closure.
Apply:
keys: [key], - limit: 200, + limit: keyLimit, disableRowLimit: true,- }, [chartConfig, setExtraFacets, dateRange, sourceType], + }, [facetsChartConfig, dateRange],Also applies to: 635-636
482-487: Remove unused sourceType prop and dependencyIt’s not referenced anymore; keep props minimal.
Apply:
sourceId, - sourceType, showDelta, @@ - sourceType?: string; @@ sourceName?: string; connectionName?: string;And drop from deps (covered above).
Also applies to: 494-500, 635-636
593-597: Hook ignores passed limit; update useGetKeyValues to honor itThe hook hardcodes limit: 1000 in useMetadata.tsx; your limit here is ineffective.
Update in packages/app/src/hooks/useMetadata.tsx:
- limit: 1000, + limit,Verification script:
#!/bin/bash rg -nP '\blimit\s*:\s*1000\b' packages/app/src/hooks/useMetadata.tsx -C2 rg -nP 'useGetKeyValues\(\s*\{' packages/app/src/components/DBSearchPageFilters.tsx -n -A6 -B2
63-73: Fix override typos/dupes that break matching
- 'SeverityText ' has a trailing space (no match).
- Duplicate "ResourceAttributes['label.pod']" in US | Traces.
Apply:
'US | Logs': [ "ResourceAttributes['cloud']", "LogAttributes['log.iostream']", - 'SeverityText ', + 'SeverityText', 'ServiceName', @@ 'US | Traces': [ @@ - "ResourceAttributes['label.pod']", - "ResourceAttributes['label.pod']", + "ResourceAttributes['label.pod']", @@ 'Sgp | Logs': [ @@ - "ResourceAttributes['label.pod']", + "ResourceAttributes['label.pod']",Also applies to: 74-85, 93-104
113-120: Normalize and guard joined-key lookup; add explicit return typePrevents misses from stray spaces/undefined parts.
Apply:
-// Helper function to get keys - override for specific types, use default for others -const getKeysForSourceType = (sourceName?: string, connectionName?: string) => { - const newSourceKey = connectionName + ' | ' + sourceName; - if (newSourceKey && newSourceKey in serviceMapOverride) { - return serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride]; - } - return []; -}; +// Helper function to get keys - override for specific types, use default for others +const getKeysForSourceType = ( + sourceName?: string, + connectionName?: string, +): string[] => { + const src = sourceName?.trim(); + const conn = connectionName?.trim(); + if (!src || !conn) return []; + const joined = `${conn} | ${src}`; + return serviceMapOverride[joined as keyof typeof serviceMapOverride] ?? []; +};
501-529: Remove debug console logging (noise + PII risk)These run in render path and should not ship.
Apply:
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); - const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = usePinnedFilters(sourceId ?? null); - - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); - const { width, startResize } = useResizable(16, 'left'); - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); @@ - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); - const [showMoreFields, setShowMoreFields] = useState(false); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error('');Also applies to: 538-547
562-566: Fallback to metadata-derived keys when no override foundPrevents “No filters available” when overrides don’t match.
Apply:
const keysToFetch = useMemo(() => { const overrides = getKeysForSourceType(sourceName, connectionName); - return overrides; -}, [sourceName, connectionName]); + return overrides.length ? overrides : fieldKeys; +}, [sourceName, connectionName, fieldKeys]);Also applies to: 549-560
🧹 Nitpick comments (5)
packages/app/src/components/DBSearchPageFilters.tsx (3)
32-112: Type the override map to catch mistakes at compile timeAdds safety and avoids silent mismatches.
Apply:
-const serviceMapOverride = { +const serviceMapOverride: Record<string, string[]> = {
625-627: Remove console in catch; surface errors via UI/telemetry if neededConsole noise in UI path; keep UX clean.
Apply:
- } catch (error) { - console.error('failed to fetch more keys', error); + } catch (error) { + // Swallow fetch error; consider toast/telemetry if desired.
32-112: Run Prettier/ESLint after changesEnsures consistent quotes/spacing and catches any now-unused imports.
Run:
#!/bin/bash npx prettier --write packages/app/src/components/DBSearchPageFilters.tsx npx eslint packages/app/src/components/DBSearchPageFilters.tsx --fix rg -nP --hidden --no-ignore '\bconsole\.' packages/app/src/components/DBSearchPageFilters.tsx || echo "✅ No console statements found." rg -nP 'useExplainQuery' packages/app/src/components/DBSearchPageFilters.tsx || echo "✅ useExplainQuery removed."Also applies to: 113-120, 501-529
packages/app/src/components/DBRowTable.tsx (2)
530-531: Respect thewrapLinesprop; avoid unexpected default and dead prop.Hard-coding
truechanges UX to wrapped lines by default and ignores thewrapLinesprop (the caller passeswrapLines={false}at Line 1162). Initialize from the prop so parents can control the initial state.- const [wrapLinesEnabled, setWrapLinesEnabled] = useState(true); + const [wrapLinesEnabled, setWrapLinesEnabled] = useState(wrapLines);If you intend to make wrapping always-on globally, consider removing the
wrapLinesprop fromRawLogTableand its call sites to prevent API drift.
692-697: Add accessible label for the wrap-lines toggle.Icon-only button lacks an accessible name; add
aria-labelortitlefor a11y and better UX.- <UnstyledButton - onClick={() => setWrapLinesEnabled(prev => !prev)} - > + <UnstyledButton + aria-label="Toggle line wrapping" + title="Toggle line wrapping" + onClick={() => setWrapLinesEnabled(prev => !prev)} + >
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/app/src/components/DBRowTable.tsx(1 hunks)packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)packages/app/src/components/SearchTotalCountChart.tsx(2 hunks)packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useChartConfig.tsx(2 hunks)packages/app/src/hooks/useMetadata.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/app/src/components/SearchTotalCountChart.tsx (1)
packages/app/src/config.ts (1)
HDX_METADATA_MAX_ROWS_TO_READ(15-17)
packages/app/src/hooks/useMetadata.tsx (1)
packages/app/src/config.ts (1)
HDX_METADATA_MAX_ROWS_TO_READ(15-17)
packages/app/src/components/DBSearchPageFilters.tsx (5)
packages/common-utils/src/types.ts (1)
ChartConfigWithDateRange(419-419)packages/app/src/searchFilters.tsx (2)
FilterStateHook(220-220)usePinnedFilters(278-352)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (2)
useAllFields(83-125)useGetKeyValues(155-204)packages/app/src/utils.ts (1)
mergePath(649-655)
🪛 ESLint
packages/app/src/config.ts
[error] 16-16: Insert ,
(prettier/prettier)
🔇 Additional comments (5)
packages/app/src/components/SearchTotalCountChart.tsx (1)
30-34: LGTM: Proper configuration override for metadata read limitsThe implementation correctly uses the new
HDX_METADATA_MAX_ROWS_TO_READconfiguration to override ClickHouse settings specifically for total count queries. The comment clearly indicates this is an API-specific override, and the pattern aligns well with the broader PR's approach to configurable metadata limits.packages/app/src/hooks/useMetadata.tsx (2)
104-106: LGTM: Consistent global metadata configurationThe refactoring from per-team conditional metadata settings to using the global
HDX_METADATA_MAX_ROWS_TO_READconfiguration provides better consistency and maintainability.
183-185: LGTM: Consistent with useAllFields approachThe metadata configuration follows the same pattern established in
useAllFields, providing consistent behavior across the codebase.packages/app/src/hooks/useChartConfig.tsx (2)
24-24: LGTM: Clean interface extension for ClickHouse settingsThe addition of the optional
clickhouseSettingsparameter provides a flexible way to pass per-query ClickHouse settings without breaking existing usage.
54-54: LGTM: Proper forwarding of ClickHouse settingsThe implementation correctly passes the optional clickhouseSettings to the ClickHouse client, maintaining the chain from API call to database query configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/app/src/config.ts (1)
14-14: Default HDX_AUTH_EMAILS to a safe JSON string to prevent JSON.parse crashesWithout a fallback, downstream
JSON.parsecan throw when the env is unset or empty. Use a trimmed default.-export const HDX_AUTH_EMAILS = env('NEXT_PUBLIC_HDX_AUTH_EMAILS'); +export const HDX_AUTH_EMAILS = + env('NEXT_PUBLIC_HDX_AUTH_EMAILS')?.trim() || '[]';Run to confirm the hook guards parsing:
#!/bin/bash rg -nC2 --color=never "useAuthEmails|HDX_AUTH_EMAILS|JSON\.parse" packages/app/src
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/config.ts(1 hunks)
| export const HDX_METADATA_MAX_ROWS_TO_READ = Number( | ||
| env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden numeric parsing for HDX_METADATA_MAX_ROWS_TO_READ (avoid NaN/0 on empty/malformed values)
Number('') -> 0 and Number('abc') -> NaN; ?? '31000000' won't catch empty strings. Add validation and a sane fallback.
-export const HDX_METADATA_MAX_ROWS_TO_READ = Number(
- env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000',
-);
+export const HDX_METADATA_MAX_ROWS_TO_READ = (() => {
+ const raw = env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ');
+ const n = Number(raw);
+ const fallback = 31_000_000;
+ return Number.isFinite(n) && n > 0 ? n : fallback;
+})();📝 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.
| export const HDX_METADATA_MAX_ROWS_TO_READ = Number( | |
| env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000', | |
| ); | |
| export const HDX_METADATA_MAX_ROWS_TO_READ = (() => { | |
| const raw = env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ'); | |
| const n = Number(raw); | |
| const fallback = 31_000_000; | |
| return Number.isFinite(n) && n > 0 ? n : fallback; | |
| })(); |
🤖 Prompt for AI Agents
In packages/app/src/config.ts around lines 15-17, the current Number(...)
parsing will convert empty strings to 0 and malformed strings to NaN and the
nullish coalescing won't catch empty values; change the export to robustly parse
and validate the env var: read the raw env string, if it's non-empty use
parseInt(raw, 10) (or Number) then check Number.isFinite(value) && value > 0,
otherwise fall back to the default 31000000; ensure the exported constant is an
integer (Math.floor if necessary) and document/handle when the env is invalid by
using the default.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/app/src/components/DBRowJsonViewer.tsx (2)
148-156: Do not mutate props: clone before filtering.You’re deleting keys directly from
data, which can cause subtle bugs if the caller reuses the same object. Clone/filter first, then pass to the recursive filter.const rowData = useMemo(() => { if (!data) { return null; } - // remove internal aliases (keys that start with __hdx_) - Object.keys(data).forEach(key => { - if (key.startsWith('__hdx_')) { - delete data[key]; - } - }); - - return filterObjectRecursively(data, debouncedFilter); + // remove internal aliases (keys that start with __hdx_) without mutating props + const cleaned = Object.fromEntries( + Object.entries(data).filter(([k]) => !k.startsWith('__hdx_')), + ); + return filterObjectRecursively(cleaned, debouncedFilter); }, [data, debouncedFilter]);
206-233: Unsafe SQL interpolation; escape strings and handle JSON columns.Directly embedding string values can break the query or enable injection if values contain quotes. Also, JSON columns likely need
toString(...)for equality.Apply this diff to build a safe, typed WHERE:
onClick: () => { - let defaultWhere = `${fieldPath} = ${ - typeof value === 'string' ? `'${value}'` : value - }`; - - // FIXME: TOTAL HACK - if (fieldPath == 'Timestamp' || fieldPath == 'TimestampTime') { - defaultWhere = `${fieldPath} = parseDateTime64BestEffort('${value}', 9)`; - } + const targetField = isJsonColumn ? `toString(${fieldPath})` : fieldPath; + const literal = + typeof value === 'string' ? sqlStringLiteral(String(value)) : String(value); + let defaultWhere = `${targetField} = ${literal}`; + // Timestamp fields: preserve existing behavior but with proper escaping + if ( + (fieldPath === 'Timestamp' || fieldPath === 'TimestampTime') && + typeof value === 'string' + ) { + defaultWhere = `${fieldPath} = parseDateTime64BestEffort(${sqlStringLiteral( + String(value), + )}, 9)`; + } router.push( generateSearchUrl({ where: defaultWhere, whereLanguage: 'sql', }), ); },Add this helper somewhere top-level in this file:
function sqlStringLiteral(v: string): string { // SQL standard: escape single quotes by doubling them return `'${v.replace(/'/g, "''")}'`; }packages/app/src/searchFilters.tsx (1)
21-36: Escape values when constructing SQL fragmentsValues are interpolated into SQL without escaping, which can break queries (e.g., O'Brien) and risks injection if any value/key is user-controlled. At minimum, escape single quotes before joining.
Apply within this range:
- condition: `${key} IN (${Array.from(values.included) - .map(v => `'${v}'`) - .join(', ')})`, + condition: `${key} IN (${Array.from(values.included) + .map(sqlQuote) + .join(', ')})`, ... - condition: `${key} NOT IN (${Array.from(values.excluded) - .map(v => `'${v}'`) - .join(', ')})`, + condition: `${key} NOT IN (${Array.from(values.excluded) + .map(sqlQuote) + .join(', ')})`,Add once near the top of the file:
const sqlQuote = (v: string) => `'${String(v).replace(/'/g, "''")}'`;
🧹 Nitpick comments (6)
packages/app/src/AutocompleteInput.tsx (4)
284-289: Confirm post-Enter behavior: suggestions won’t reopen while focused.After
setIsInputDropdownOpen(false), suggestions only reopen on blur→focus, not on continued typing. If you want them to reappear when the user resumes typing, consider reopening on change.Apply this diff to reopen on typing:
- onChange={e => onChange(e.target.value)} + onChange={e => { + const next = e.target.value; + onChange(next); + if ( + isSearchInputFocused && + !isInputDropdownOpen && + (showSuggestionsOnEmpty || next.trim().length > 0) + ) { + setIsInputDropdownOpen(true); + } + }}
284-286: Trim before writing to history to avoid dupes/whitespace entries.Persisting raw
valuecan create duplicates like"foo"vs"foo "and allow empty strings.Apply this diff:
- if (queryHistoryType && value) { - setQueryHistory(value); - } + const trimmed = value?.trim(); + if (queryHistoryType && trimmed) { + setQueryHistory(trimmed); + }
274-289: Consider preventing native form submit on Enter to avoid double-submit.If this input lives inside a
<form>, pressing Enter may trigger both the form submit andonSubmit. Guard withpreventDefault()where you handle the key.Apply this diff:
if (e.key === 'Enter' && e.target instanceof HTMLInputElement) { if ( suggestedProperties.length > 0 && selectedAutocompleteIndex < suggestedProperties.length && selectedAutocompleteIndex >= 0 ) { + e.preventDefault(); // avoid form submit when accepting suggestion onAcceptSuggestion( suggestedProperties[selectedAutocompleteIndex].value, ); } else { + e.preventDefault(); // avoid native form submit; delegate to onSubmit if (queryHistoryType && value) { - setQueryHistory(value); + setQueryHistory(value); } setIsInputDropdownOpen(false); // Close suggestion box on Enter onSubmit?.(); } }
134-141: onToggle guard vs. explicit close—clarify single source of truth.
onToggleignores close while focused, but the Enter path explicitly closes the dropdown, bypassing that guard. Consider removing the guard or adding a comment to document why both patterns coexist.packages/app/src/components/DBRowJsonViewer.tsx (2)
24-51: Filtering alters array structure and ignores non-string primitives.
filterObjectRecursivelyconverts arrays into objects and only matches string values. This can surprise users (e.g., arrays of numbers) and change shapes.
- Preserve arrays with
Array.isArray(value)and filter their elements.- Consider matching
String(value)for numbers/booleans.I can provide a drop-in function if you want it.
285-295: Clipboard ops: handle promise rejections.
navigator.clipboard.writeTextreturns a Promise; failures (permissions/HTTP) should notify the user.- window.navigator.clipboard.writeText( - JSON.stringify(copiedObj, null, 2), - ); - notifications.show({ color: 'green', message: `Copied object to clipboard` }); + window.navigator.clipboard + .writeText(JSON.stringify(copiedObj, null, 2)) + .then(() => + notifications.show({ color: 'green', message: 'Copied object to clipboard' }), + ) + .catch(() => + notifications.show({ color: 'red', message: 'Failed to copy to clipboard' }), + );- window.navigator.clipboard.writeText( - typeof value === 'string' ? value : JSON.stringify(value, null, 2), - ); - notifications.show({ color: 'green', message: `Value copied to clipboard` }); + window.navigator.clipboard + .writeText(typeof value === 'string' ? value : JSON.stringify(value, null, 2)) + .then(() => + notifications.show({ color: 'green', message: 'Value copied to clipboard' }), + ) + .catch(() => + notifications.show({ color: 'red', message: 'Failed to copy to clipboard' }), + );Also applies to: 303-318
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/app/src/AutocompleteInput.tsx(1 hunks)packages/app/src/components/DBRowJsonViewer.tsx(1 hunks)packages/app/src/searchFilters.tsx(1 hunks)
🔇 Additional comments (3)
packages/app/src/AutocompleteInput.tsx (1)
284-289: Dropdown now closes on Enter — good UX polish.Closing the suggestions popover before firing
onSubmitavoids leftover UI and feels snappier.packages/app/src/searchFilters.tsx (2)
124-130: Reset-on-empty behavior change: LGTM; confirm UX expectationsRemoving the non-empty guard ensures URL-clears sync the in-memory state to {}. This should prevent stale chips when route/query changes. Please verify flows: clearing query params, switching source/connection, and back/forward navigation behave as intended (local selections are reset when URL has no filters).
146-178: EnableMapSet already configured
enableMapSet()is imported and called inpackages/app/pages/_app.tsx(line 38), so Set mutations inproducework correctly.
| // if ( | ||
| // onPropertyAddClick != null && | ||
| // typeof value === 'string' && | ||
| // value && | ||
| // fieldPath != 'Timestamp' && | ||
| // fieldPath != 'TimestampTime' | ||
| // ) { | ||
| // actions.push({ | ||
| // key: 'add-to-search', | ||
| // label: ( | ||
| // <> | ||
| // <i className="bi bi-funnel-fill me-1" /> | ||
| // Add to Filters | ||
| // </> | ||
| // ), | ||
| // title: 'Add to Filters', | ||
| // onClick: () => { | ||
| // onPropertyAddClick( | ||
| // isJsonColumn ? `toString(${fieldPath})` : fieldPath, | ||
| // value, | ||
| // ); | ||
| // notifications.show({ | ||
| // color: 'green', | ||
| // message: `Added "${fieldPath} = ${value}" to filters`, | ||
| // }); | ||
| // }, | ||
| // }); | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented-out “Add to Filters”: remove dead code and related deps.
If the feature is intentionally retired (moved to the new source-type filters), delete the block and drop onPropertyAddClick from the context destructure and dependency array to avoid churn/re-renders and lint noise.
- const {
- onPropertyAddClick,
- generateSearchUrl,
- generateChartUrl,
- displayedColumns,
- toggleColumn,
- } = useContext(RowSidePanelContext);
+ const { generateSearchUrl, generateChartUrl, displayedColumns, toggleColumn } =
+ useContext(RowSidePanelContext);- // Add to Filters action (strings only)
- // FIXME: TOTAL HACK To disallow adding timestamp to filters
- // if (
- // onPropertyAddClick != null &&
- // typeof value === 'string' &&
- // value &&
- // fieldPath != 'Timestamp' &&
- // fieldPath != 'TimestampTime'
- // ) {
- // actions.push({
- // key: 'add-to-search',
- // label: (
- // <>
- // <i className="bi bi-funnel-fill me-1" />
- // Add to Filters
- // </>
- // ),
- // title: 'Add to Filters',
- // onClick: () => {
- // onPropertyAddClick(
- // isJsonColumn ? `toString(${fieldPath})` : fieldPath,
- // value,
- // );
- // notifications.show({
- // color: 'green',
- // message: `Added "${fieldPath} = ${value}" to filters`,
- // });
- // },
- // });
- // } }, [
displayedColumns,
generateChartUrl,
generateSearchUrl,
- onPropertyAddClick,
rowData,
toggleColumn,
jsonColumns,
]);Run to confirm nothing else depends on it:
Also applies to: 132-139, 322-331
🏁 Script executed:
#!/bin/bash
rg -nC2 '\bonPropertyAddClick\b|Add to Filters'Length of output: 12087
Remove dead “Add to Filters” code and clean up DBRowJsonViewer context
- Delete the commented-out Add to Filters block (lines 175–193) and drop
onPropertyAddClickfrom theuseContext(RowSidePanelContext)destructure (lines 132–139) and effect dependency array (lines 322–331). - Update DBRowJsonViewer.test.tsx to remove or adjust the test asserting on “Add to Filters” and calls to
onPropertyAddClick.
🤖 Prompt for AI Agents
In packages/app/src/components/DBRowJsonViewer.tsx around lines 175–193 and the
context destructure at ~132–139 and effect deps at ~322–331, remove the entire
commented-out “Add to Filters” block (lines 175–193), delete onPropertyAddClick
from the RowSidePanelContext destructure and from the effect dependency array,
and then update packages/app/src/components/__tests__/DBRowJsonViewer.test.tsx
to remove or adjust any assertions and mock calls related to “Add to Filters” /
onPropertyAddClick so tests no longer reference that prop or UI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
packages/app/src/components/DBSearchPageFilters.tsx (8)
267-272: Avoid eager “Load more” for every facet (perf/cost).Auto-triggering on mount fans out N extra requests. Gate by pin state (already passed in as prop).
-useEffect(() => { - if (onLoadMore && !hasLoadedMore) { - onLoadMore(name); - } -}, [onLoadMore, hasLoadedMore, name]); +useEffect(() => { + if (onLoadMore && !hasLoadedMore && isFieldPinned) { + onLoadMore(name); + } +}, [onLoadMore, hasLoadedMore, name, isFieldPinned]);
530-531: Remove unused/expensive useExplainQuery.It’s solely logged (and logs are being removed). Drop the import and hook to avoid an extra EXPLAIN query.
- import { useExplainQuery } from '@/hooks/useExplainQuery';- const { data: countData } = useExplainQuery(chartConfig);Also applies to: 22-22
623-629: Fix callback deps: remove unused, add facetsChartConfig.Avoid stale config capture; remove unrelated deps and stable setters.
- chartConfig: { - ...facetsChartConfig, - dateRange, - }, + chartConfig: { ...facetsChartConfig, dateRange },- }, [chartConfig, setExtraFacets, dateRange, sourceType]); + }, [facetsChartConfig, dateRange]);Also applies to: 647-648
501-548: Strip debug console logging (noise + potential PII leakage).Large console.error blocks in render path. Remove or gate behind dev-only logger.
- // Log all component props and variables - console.error('🚀 DBSearchPageFiltersComponent - ALL PROPS AND VARIABLES:'); - console.error('📋 Main Props:'); - console.error(' - analysisMode:', analysisMode); - console.error(' - isLive:', isLive); - console.error(' - sourceId:', sourceId); - console.error(' - sourceType:', sourceType); - console.error(' - showDelta:', showDelta); - console.error(' - denoiseResults:', denoiseResults); - console.error(' - chartConfig:', chartConfig); - console.error('📊 FilterStateHook Props:'); - console.error(' - filters (filterState):', filterState); - console.error(' - clearAllFilters function:', typeof clearAllFilters); - console.error(' - clearFilter function:', typeof clearFilter); - console.error(' - setFilterValue function:', typeof setFilterValue); - console.error('🔧 Hook Results:'); - const { toggleFilterPin, toggleFieldPin, isFilterPinned, isFieldPinned } = usePinnedFilters(sourceId ?? null); - - console.error(' - toggleFilterPin function:', typeof toggleFilterPin); - console.error(' - toggleFieldPin function:', typeof toggleFieldPin); - console.error(' - isFilterPinned function:', typeof isFilterPinned); - console.error(' - isFieldPinned function:', typeof isFieldPinned); - const { width, startResize } = useResizable(16, 'left'); - console.error(' - width:', width); - console.error(' - startResize function:', typeof startResize); - - const { data: countData } = useExplainQuery(chartConfig); @@ - console.error(' - countData:', countData); - console.error(' - allFields data:', data); - console.error(' - allFields isLoading:', isLoading); - console.error('📊 State Variables:'); - const [showMoreFields, setShowMoreFields] = useState(false); - console.error(' - showMoreFields:', showMoreFields); - console.error(' - setShowMoreFields function:', typeof setShowMoreFields); - console.error('🚀 END OF COMPONENT PROPS LOGGING 🚀'); - console.error('');
549-566: Use metadata-derived fieldKeys as fallback when overrides are empty.Prevents “No filters available” when no override match exists.
-const keysToFetch = useMemo(() => { - const overrides = getKeysForSourceType(sourceName, connectionName); - return overrides; -}, [sourceName, connectionName]); +const keysToFetch = useMemo(() => { + const overrides = getKeysForSourceType(sourceName, connectionName); + return overrides.length ? overrides : fieldKeys; +}, [sourceName, connectionName, fieldKeys]);
63-85: Fix bad override entries (trailing space + duplicate key).
- 'SeverityText ' has a trailing space (won’t match).
- "ResourceAttributes['label.pod']" is duplicated.
'US | Logs': [ @@ - 'SeverityText ', + 'SeverityText', @@ 'US | Traces': [ @@ - "ResourceAttributes['label.pod']", - "ResourceAttributes['label.pod']", + "ResourceAttributes['label.pod']",
606-609: Replace hardcoded limit with passedlimitparameter inuseGetKeyValues
Hook currently ignores thelimitargument and always useslimit: 1000(useMetadata.tsx:192). Update as follows:packages/app/src/hooks/useMetadata.tsx - limit: 1000, + limit,
113-121: Harden getKeysForSourceType: type, trim/guard, and normalized join.Prevents undefined/whitespace mismatches and clarifies return type. Also allows a fallback to sourceName-only keys in case connectionName is absent.
-// Helper function to get keys - override for specific types, use default for others -const getKeysForSourceType = (sourceName?: string, connectionName?: string) => { - const newSourceKey = connectionName + ' | ' + sourceName; - if (newSourceKey && newSourceKey in serviceMapOverride) { - return serviceMapOverride[newSourceKey as keyof typeof serviceMapOverride]; - } - return []; -}; +// Helper function to get keys - override for specific types, use default for others +const getKeysForSourceType = ( + sourceName?: string, + connectionName?: string, +): string[] => { + const src = sourceName?.trim(); + const conn = connectionName?.trim(); + if (src && conn) { + const joined = `${conn} | ${src}`; + if (joined in serviceMapOverride) { + return serviceMapOverride[joined as keyof typeof serviceMapOverride]; + } + } + // Fallback: allow direct sourceName key match, else empty list + if (src && src in serviceMapOverride) { + return serviceMapOverride[src as keyof typeof serviceMapOverride]; + } + return []; +};
🧹 Nitpick comments (2)
packages/app/src/components/DBSearchPageFilters.tsx (2)
482-500: Remove unused sourceType prop (not referenced after cleanup).It’s only present in logs and a stray callback dep. Drop it to avoid prop drift.
sourceId, - sourceType, showDelta, @@ - sourceType?: string; @@ sourceName?: string; connectionName?: string;If other files still pass sourceType, update call sites accordingly.
579-590: Effect dependencies: avoid stringify churn.JSON.stringify on where/filters risks frequent resets and perf hits. Prefer a stable hash or deep-compare effect.
Example: compute a memoized key once and depend on that.
const whereKey = useMemo(() => JSON.stringify(chartConfig.where), [chartConfig.where]); const filtersKey = useMemo(() => JSON.stringify(chartConfig.filters), [chartConfig.filters]); useEffect(() => { setExtraFacets({}); }, [chartConfig.from.tableName, chartConfig.from.databaseName, chartConfig.connection, whereKey, filtersKey]);
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/app/src/components/DBSearchPageFilters.tsx(9 hunks)packages/app/src/hooks/useMetadata.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/hooks/useMetadata.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/components/DBSearchPageFilters.tsx (5)
packages/common-utils/src/types.ts (1)
ChartConfigWithDateRange(419-419)packages/app/src/searchFilters.tsx (2)
FilterStateHook(217-217)usePinnedFilters(275-349)packages/app/src/hooks/useExplainQuery.tsx (1)
useExplainQuery(8-36)packages/app/src/hooks/useMetadata.tsx (2)
useAllFields(83-125)useGetKeyValues(155-203)packages/app/src/utils.ts (1)
mergePath(649-655)
🔇 Additional comments (4)
packages/app/src/components/DBSearchPageFilters.tsx (4)
156-158: LGTM: noop handler to prevent double fire.Clear intent and avoids duplicate onChange triggers.
338-338: Good catch: include isPinned in deps.Prevents stale closures in sorting logic.
593-601: LGTM: facetsChartConfig removes orderBy.Keeps value distribution unbiased for facet lookup.
650-691: LGTM: facet merging and pin-first ordering.Merges “load more” values safely and prioritizes pinned fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/app/src/components/ContextSidePanel.tsx (3)
83-90: Fix Prettier violations and make Pod default more robust.
- Prettier: remove trailing spaces (Line 83, Line 86) and add the trailing comma Prettier expects (Line 89).
- Optional: if
rowDatacan change without a remount, sync the default when a pod becomes available.Apply:
- const [contextBy, setContextBy] = useState<ContextBy>( - k8sPodName ? ContextBy.Pod : ContextBy.All - ); + const [contextBy, setContextBy] = useState<ContextBy>( + k8sPodName ? ContextBy.Pod : ContextBy.All, + );If
rowDatacan change, consider:// keep defaulting to Pod when available and user hasn't chosen something else useEffect(() => { if (k8sPodName && contextBy === ContextBy.All) setContextBy(ContextBy.Pod); }, [k8sPodName, contextBy]);
151-179: Prefer scoping by Pod UID (fallback to name) and guard undefined values.Using
k8s.pod.uidavoids ambiguity across restarts/rollouts; use it when present and fall back to name. Also ensure we never build a clause with an empty value.Apply:
@@ - [ContextBy.Pod]: { - field: 'k8s.pod.name', - value: k8sPodName, - }, + [ContextBy.Pod]: { + field: k8sPodUid ? 'k8s.pod.uid' : 'k8s.pod.name', + value: k8sPodUid ?? k8sPodName, + }, @@ - [k8sNodeName, k8sPodName, host, service, debouncedWhere], + [k8sNodeName, k8sPodName, k8sPodUid, host, service, debouncedWhere],And (paired change above in the earlier block):
- // Extract pod name to determine initial context - const k8sPodName = rowData.ResourceAttributes?.['k8s.pod.name']; + // Extract pod identifiers to determine initial context + const k8sPodName = rowData.ResourceAttributes?.['k8s.pod.name']; + const k8sPodUid = rowData.ResourceAttributes?.['k8s.pod.uid'];Additionally, update the SegmentedControl data builder to show “Pod” if either UID or name exists:
...(k8sPodUid || k8sPodName ? [{ label: 'Pod', value: ContextBy.Pod }] : []),Finally, inside
getWhereClause, consider short‑circuiting when!mapping.valueto avoid producing invalid clauses.
148-149: Comment clarity.If you adopt the UID fallback, update this comment to reflect both
k8sPodUidandk8sPodNameextraction.
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/components/ContextSidePanel.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/components/ContextSidePanel.tsx (1)
packages/app/src/components/DBInfraPanel.tsx (1)
rowData(206-277)
🪛 ESLint
packages/app/src/components/ContextSidePanel.tsx
[error] 83-83: Delete ··
(prettier/prettier)
[error] 86-86: Delete ··
(prettier/prettier)
[error] 89-89: Insert ,
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
288-307: Fix next-offset calculation to avoid duplicates when maxPages trims earlier pages.Using flattenPages(allPages).length under maxPages can undercount and cause overlapping pages. Advance by lastPageParam + rows returned.
- getNextPageParam: (lastPage, allPages) => { + getNextPageParam: (lastPage, allPages, lastPageParam) => { if (lastPage == null) { return undefined; } const len = lastPage.data.length; if (len === 0) { return undefined; } - // If we got less data than the page size, we've reached the end + // If we got less data than the page size, we've reached the end if (len < pageSize) { return undefined; } - const data = flattenPages(allPages); - - // Next offset is the total number of records fetched so far - return data.length; + // Next offset advances by the rows we just fetched + return (lastPageParam ?? 0) + len; },
🧹 Nitpick comments (4)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (4)
46-46: Strongly type meta instead of using any; guard pageSize.Casting meta to any hides errors and risks undefined/NaN pageSize flowing into limits.
Apply this diff:
- const pageSize = (meta as any).pageSize as number; + const { pageSize } = meta as QueryMeta;Add this type near the imports (outside the shown range):
type QueryMeta = { queryClient: QueryClient; hasPreviousQueries: boolean; pageSize: number; };
57-59: Clamp pageSize before injecting into SQL limit.Prevents accidental 0/negative/float values.
- limit: pageSize, + // Ensure a sane integer limit + limit: Math.max(1, Math.floor(pageSize)),
248-249: Name the default as a constant for discoverability.Minor ergonomics; makes the magic number self-documenting.
- pageSize = 3400, + pageSize = DEFAULT_PAGE_SIZE,Add near the top (outside this hunk):
const DEFAULT_PAGE_SIZE = 3400 as const;
72-75: Use the existing HDX_METADATA_MAX_ROWS_TO_READ constant instead of a hard-coded valueReplace the string literal in clickhouse_settings with the imported constant, converting it to a string for consistency with other usages:
- clickhouse_settings: { - max_rows_to_read: '91000000', - }, + clickhouse_settings: { + max_rows_to_read: HDX_METADATA_MAX_ROWS_TO_READ.toString(), + },And at the top of the file add:
import { HDX_METADATA_MAX_ROWS_TO_READ } from '@/config';
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx(5 hunks)
🔇 Additional comments (3)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (3)
298-301: End-of-data guard looks good.Stopping when last page < pageSize is correct; with the offset fix above this will avoid an extra empty fetch.
312-313: Meta propagation LGTM.Passing pageSize and hasPreviousQueries via meta cleanly connects options to queryFn.
253-254: Public API change: addedpageSizeparameter (default 3400) touseOffsetPaginatedQuery—verify call sites.Review all hook invocations (e.g. in DBTableChart.tsx, DBRowTable.tsx, DBTraceWaterfallChart.tsx) to ensure callers don’t rely on the previous implicit server-side limits, and update the hook’s documentation to state the new default page size.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
249-249: Consider the impact of the default page size on performance.The default
HDX_DEFAULT_PAGE_SIZEof 3,400 rows per page is relatively large. This could impact initial load times and memory usage, especially for users on slower connections or devices.Consider documenting the rationale for this default value and ensure it's appropriate for your typical use cases. You might want to make this configurable based on the type of query or user preferences.
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/app/src/config.ts(1 hunks)packages/app/src/hooks/useOffsetPaginatedQuery.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
packages/app/src/config.ts (2)
HDX_METADATA_MAX_ROWS_TO_READ(15-17)HDX_DEFAULT_PAGE_SIZE(18-20)
🪛 ESLint
packages/app/src/hooks/useOffsetPaginatedQuery.tsx
[error] 1-21: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (3)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (3)
305-307: LGTM! Offset calculation is correct.The offset calculation using
data.lengthafter flattening all pages correctly represents the total number of records fetched, which aligns well with the new pagination termination logic.
73-76: Verify ClickHousemax_rows_to_readoverride. This hook now setsmax_rows_to_readtoHDX_METADATA_MAX_ROWS_TO_READ(~31 M), overriding the client-lib default of 10 M (common-utils/src/clickhouse.ts:468) and the metadata default of 3 M (common-utils/src/metadata.ts:287). Confirm your ClickHouse servers support this limit without degrading performance or exceeding resource quotas.
299-303: Add pagination edge-case tests for useOffsetPaginatedQuery
No existing tests cover when total rows % pageSize === 0; verify that theif (len < pageSize)termination correctly handles both “less than” and “exactly divisible” scenarios.
| throw new Error('Query missing client meta'); | ||
| } | ||
| const queryClient = meta.queryClient as QueryClient; | ||
| const pageSize = (meta as any).pageSize as number; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for pageSize extraction from meta.
The current type assertion (meta as any).pageSize as number bypasses TypeScript's type system. Consider adding proper typing to the meta object to ensure type safety.
Consider extending the meta type properly instead of using any:
- const pageSize = (meta as any).pageSize as number;
+ const pageSize = (meta as { queryClient: QueryClient; hasPreviousQueries: boolean; pageSize: number }).pageSize;Alternatively, create a custom type for the meta object that can be reused across the file.
📝 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.
| const pageSize = (meta as any).pageSize as number; | |
| const pageSize = (meta as { queryClient: QueryClient; hasPreviousQueries: boolean; pageSize: number }).pageSize; |
🤖 Prompt for AI Agents
In packages/app/src/hooks/useOffsetPaginatedQuery.tsx around line 47, the code
unsafely extracts pageSize using (meta as any).pageSize as number; replace this
with a proper meta type: declare an interface (e.g. PaginatedMeta { pageSize?:
number; total?: number; cursor?: string; }) and use it in the query/useQuery
generics or local variable typing so meta is typed as PaginatedMeta; then access
pageSize via const pageSize = (meta as PaginatedMeta).pageSize ??
DEFAULT_PAGE_SIZE and add a runtime check to ensure it's a number (e.g. typeof
pageSize === 'number' ? pageSize : DEFAULT_PAGE_SIZE). Make the interface
reusable by exporting it from the hook file (or a shared types file) so other
code can use the same typed meta.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (2)
46-59: Type meta and validate pageSize; avoid any and NaN/≤0 values.
(meta as any).pageSizerisks NaN/negative values leading to runaway pagination. Use a typed meta and derive aneffectivePageSize; also use it in the query limit.- const queryClient = meta.queryClient as QueryClient; - const pageSize = (meta as any).pageSize as number; + const { queryClient, pageSize: metaPageSize } = meta as { + queryClient: QueryClient; + hasPreviousQueries: boolean; + pageSize?: number; + }; + const mps = typeof metaPageSize === 'number' ? metaPageSize : NaN; + const effectivePageSize = + Number.isFinite(mps) && mps > 0 ? Math.floor(mps) : HDX_DEFAULT_PAGE_SIZE; @@ - limit: { - limit: pageSize, + limit: { + limit: effectivePageSize, offset: pageParam, },
243-255: AddpageSizeto the query key and sanitize its value.
- Update
queryKeyFnto accept(prefix, config, pageSize)and includepageSizein the returned tuple.- In
useOffsetPaginatedQuery, computethen callconst safeDefaultPageSize = Number.isFinite(HDX_DEFAULT_PAGE_SIZE) && HDX_DEFAULT_PAGE_SIZE > 0 ? Math.floor(HDX_DEFAULT_PAGE_SIZE) : /* fallback value */; const effectivePageSize = Number.isFinite(pageSize) && pageSize > 0 ? Math.floor(pageSize) : safeDefaultPageSize;queryKeyFn(queryKeyPrefix, config, effectivePageSize).- Update all affected generics from
[string, ChartConfigWithDateRange]to[string, ChartConfigWithDateRange, number].
♻️ Duplicate comments (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
21-21: Fix import sorting (ESLint: simple-import-sort/imports).Keep alias imports sorted; move
@/configabove@/metadataand@/utils.import { getClickhouseClient } from '@/clickhouse'; -import { getMetadata } from '@/metadata'; -import { omit } from '@/utils'; -import { HDX_METADATA_MAX_ROWS_TO_READ, HDX_DEFAULT_PAGE_SIZE } from '@/config'; +import { HDX_DEFAULT_PAGE_SIZE, HDX_METADATA_MAX_ROWS_TO_READ } from '@/config'; +import { getMetadata } from '@/metadata'; +import { omit } from '@/utils';
🧹 Nitpick comments (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
73-75: Guard ClickHouse setting from invalid env (NaN/≤0).If
HDX_METADATA_MAX_ROWS_TO_READbecomesNaNdue to a bad env, ClickHouse may reject the query. Prefer validating at definition time inconfig.ts; keepString(...)here.Proposed hardening in packages/app/src/config.ts (additional context, not in this file):
-export const HDX_METADATA_MAX_ROWS_TO_READ = Number( - env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ') ?? '31000000', -); +export const HDX_METADATA_MAX_ROWS_TO_READ = (() => { + const raw = env('NEXT_PUBLIC_HDX_METADATA_MAX_ROWS_TO_READ'); + const n = Number(raw ?? '31000000'); + return Number.isFinite(n) && n > 0 ? n : 31_000_000; +})();
📜 Review details
Configuration used: Repository: ShareChat/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/app/src/hooks/useOffsetPaginatedQuery.tsx (1)
packages/app/src/config.ts (2)
HDX_METADATA_MAX_ROWS_TO_READ(15-17)HDX_DEFAULT_PAGE_SIZE(18-20)
🪛 ESLint
packages/app/src/hooks/useOffsetPaginatedQuery.tsx
[error] 1-21: Run autofix to sort these imports!
(simple-import-sort/imports)
| // If we got less data than the page size, we've reached the end | ||
| if (len < pageSize) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const data = flattenPages(allPages); | ||
|
|
||
| // TODO: Need to configure overlap and account for granularity | ||
| // Next offset is the total number of records fetched so far | ||
| return data.length; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use effectivePageSize for end-of-data detection and offsets.
Keep EoD logic consistent with the sanitized size.
- // If we got less data than the page size, we've reached the end
- if (len < pageSize) {
+ // If we got less data than the page size, we've reached the end
+ if (len < effectivePageSize) {
return undefined;
}
@@
- // Next offset is the total number of records fetched so far
+ // Next offset is the total number of records fetched so far
return data.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 we got less data than the page size, we've reached the end | |
| if (len < pageSize) { | |
| return undefined; | |
| } | |
| const data = flattenPages(allPages); | |
| // TODO: Need to configure overlap and account for granularity | |
| // Next offset is the total number of records fetched so far | |
| return data.length; | |
| // If we got less data than the page size, we've reached the end | |
| if (len < effectivePageSize) { | |
| return undefined; | |
| } | |
| const data = flattenPages(allPages); | |
| // Next offset is the total number of records fetched so far | |
| return data.length; |
🤖 Prompt for AI Agents
In packages/app/src/hooks/useOffsetPaginatedQuery.tsx around lines 299 to 307,
the end-of-data detection and next-offset calculation use the raw pageSize and
data.length instead of the sanitized effectivePageSize and the flattened pages'
sanitized length; change the conditional to compare len (or flattened length)
against effectivePageSize to decide EoD and compute the next offset using the
total number of records counted with the same sanitization (effectivePageSize)
so both EoD detection and returned offset are consistent with the sanitized page
size.
| pageSize, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate effectivePageSize via meta.
Align meta with the sanitized value used in the query.
meta: {
queryClient,
hasPreviousQueries,
- pageSize,
+ pageSize: effectivePageSize,
},📝 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.
| pageSize, | |
| }, | |
| meta: { | |
| queryClient, | |
| hasPreviousQueries, | |
| pageSize: effectivePageSize, | |
| }, |
🤖 Prompt for AI Agents
In packages/app/src/hooks/useOffsetPaginatedQuery.tsx around lines 313-314, the
meta returned from the hook currently reflects the original pageSize instead of
the sanitized value used in the query; update the meta to include (or replace
with) effectivePageSize set to the sanitized value the query actually uses (the
computed/sanitized pageSize variable) so consumers see the true page size used
for fetching.
Summary by CodeRabbit
New Features
Chores
Tweaks