chore: Segment improvements#1971
Merged
Merged
Conversation
* Update static properties * and knowledge/setting actions * avoid duplicate events
github-actions
Bot
added
community
frontend
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAnalytics tracking was added to knowledge and settings UI actions, onboarding page-view tracking now fires once per component lifecycle, and the analytics static property set was expanded with additional required fields. ChangesAnalytics instrumentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…provements-and-coverage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/app/settings/_components/model-provider-dialog-footer.tsx (1)
27-28: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
providerKeyrequired in the footer props.Both new tracking paths unconditionally emit
payload: { provider: providerKey }, so keeping this prop optional lets future callers compile while silently sending malformed provider analytics. All call sites in this PR already pass a concrete key, so tightening the prop contract is the safer shape here.♻️ Proposed fix
- providerKey?: string; + providerKey: string;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/settings/_components/model-provider-dialog-footer.tsx` around lines 27 - 28, `providerKey` in `ModelProviderDialogFooter` is currently optional, but the footer always uses it to emit provider analytics. Update the footer props type to require `providerKey` and make sure `ModelProviderDialogFooter` callers continue passing a concrete key so the `payload: { provider: providerKey }` paths cannot compile with missing data.frontend/app/settings/_components/model-provider-card.tsx (1)
91-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the button label once and reuse it for analytics and UI.
The click handler and rendered content duplicate the same
isUnhealthy/isConfiguredbranching. Keeping one shared label avoids the UI text and tracked CTA drifting apart on the next edit.♻️ Proposed fix
export default function ModelProviderCard({ provider, isConfigured, isUnhealthy, onConfigure, }: ModelProviderCardProps) { const isCloudBrand = useIsCloudBrand(); const { providerKey, name, logo: Logo, logoColor, logoBgColor } = provider; const isEditSetup = isConfigured && !isUnhealthy; + const buttonLabel = isUnhealthy + ? "Fix Setup" + : isConfigured + ? "Edit Setup" + : "Configure"; return ( @@ } onClick={() => { - const cta = isUnhealthy - ? "Fix Setup" - : isConfigured - ? "Edit Setup" - : "Configure"; trackButton({ - CTA: `${cta} - ${name}`, + CTA: `${buttonLabel} - ${name}`, elementId: "provider-card-button", namespace: "settings", payload: { provider: providerKey }, }); onConfigure(providerKey); }} > - {isUnhealthy - ? "Fix Setup" - : isConfigured - ? "Edit Setup" - : "Configure"} + {buttonLabel} </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/settings/_components/model-provider-card.tsx` around lines 91 - 110, The button label logic in ModelProviderCard is duplicated in the onClick handler and the rendered CTA text, which can cause analytics and UI copy to drift. Derive the label once in model-provider-card.tsx using the existing isUnhealthy and isConfigured conditions, then reuse that shared value both in trackButton and in the button content so the label stays consistent.frontend/app/settings/_components/ingest-settings-section.tsx (1)
229-241: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTracking fires before validation, so aborted saves are still recorded as successful "Save Ingest Settings" events.
trackButtonruns ahead of thechunkSize/chunkOverlapchecks at Lines 242–253. When validation fails the mutation never runs, yet the event (with the full settings payload) is still emitted. If you want the event to reflect actual save attempts, move the call after the validation guards.♻️ Optional: emit after validation
const handleKnowledgeIngestSave = () => { - trackButton({ - CTA: "Save Ingest Settings", - elementId: "save-ingest-settings-button", - namespace: "settings", - payload: { - chunk_size: chunkSize, - chunk_overlap: chunkOverlap, - table_structure: tableStructure, - ocr, - picture_descriptions: pictureDescriptions, - disable_ingest_with_langflow: disableIngestWithLangflow, - }, - }); if (chunkSize < 1) { const msg = "Chunk size must be at least 1"; setChunkValidationError(msg); toast.error("Could not save ingest settings", { description: msg }); return; } if (chunkOverlap >= chunkSize) { const msg = "Chunk overlap must be less than chunk size"; setChunkValidationError(msg); toast.error("Could not save ingest settings", { description: msg }); return; } + trackButton({ + CTA: "Save Ingest Settings", + elementId: "save-ingest-settings-button", + namespace: "settings", + payload: { + chunk_size: chunkSize, + chunk_overlap: chunkOverlap, + table_structure: tableStructure, + ocr, + picture_descriptions: pictureDescriptions, + disable_ingest_with_langflow: disableIngestWithLangflow, + }, + }); updateSettingsMutation.mutate(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/settings/_components/ingest-settings-section.tsx` around lines 229 - 241, The “Save Ingest Settings” tracking in ingest-settings-section.tsx is firing before the validation guards, so failed saves are still recorded as successful attempts. Move the trackButton call in the settings save flow to after the chunkSize/chunkOverlap validation checks in the relevant save handler so only validated save attempts emit the event, and keep the existing payload fields tied to the actual mutation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/components/chat-renderer.tsx`:
- Around line 33-34: The onboarding page-view guard is currently module-global,
so once ChatRenderer tracks the event it blocks all future onboarding views in
the same runtime. Move the “already tracked” state into the onboarding
session/instance handled by ChatRenderer, or explicitly reset it when onboarding
ends so OpenRAG - Onboarding Page Viewed can fire again for later legitimate
entries.
In `@frontend/components/knowledge-actions-dropdown.tsx`:
- Around line 142-147: The analytics payload in knowledge-actions-dropdown’s
trackButton calls is sending the user-controlled filename, which can expose
sensitive document metadata. Update the click tracking in the View Chunks flow
and the similar tracking block referenced in the component to remove raw
filenames from payloads, and instead use coarse metadata or a non-reversible
surrogate if document-level correlation is needed. Keep the fix localized to the
trackButton invocations in knowledge-actions-dropdown so the Segment event still
records the CTA and elementId without leaking filename.
In `@frontend/components/knowledge-search-input.tsx`:
- Around line 29-34: The analytics payload in the search button tracking
currently includes raw user input from searchQueryInput.trim(), which should not
be sent to Segment. Update the trackButton call in knowledge-search-input.tsx to
remove the free-form query text and instead send only non-sensitive metadata
such as query length, whether the query is empty, or related filter state, using
the existing search handling around trackButton and searchQueryInput as the
place to change.
---
Nitpick comments:
In `@frontend/app/settings/_components/ingest-settings-section.tsx`:
- Around line 229-241: The “Save Ingest Settings” tracking in
ingest-settings-section.tsx is firing before the validation guards, so failed
saves are still recorded as successful attempts. Move the trackButton call in
the settings save flow to after the chunkSize/chunkOverlap validation checks in
the relevant save handler so only validated save attempts emit the event, and
keep the existing payload fields tied to the actual mutation path.
In `@frontend/app/settings/_components/model-provider-card.tsx`:
- Around line 91-110: The button label logic in ModelProviderCard is duplicated
in the onClick handler and the rendered CTA text, which can cause analytics and
UI copy to drift. Derive the label once in model-provider-card.tsx using the
existing isUnhealthy and isConfigured conditions, then reuse that shared value
both in trackButton and in the button content so the label stays consistent.
In `@frontend/app/settings/_components/model-provider-dialog-footer.tsx`:
- Around line 27-28: `providerKey` in `ModelProviderDialogFooter` is currently
optional, but the footer always uses it to emit provider analytics. Update the
footer props type to require `providerKey` and make sure
`ModelProviderDialogFooter` callers continue passing a concrete key so the
`payload: { provider: providerKey }` paths cannot compile with missing data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66a2aab4-b17e-4ba4-8002-e722a8ad17a7
📒 Files selected for processing (20)
frontend/app/knowledge/chunks/page.tsxfrontend/app/knowledge/page.tsxfrontend/app/settings/_components/agent-settings-section.tsxfrontend/app/settings/_components/anthropic-settings-dialog.tsxfrontend/app/settings/_components/api-keys-section.tsxfrontend/app/settings/_components/connector-access-section.tsxfrontend/app/settings/_components/connector-card.tsxfrontend/app/settings/_components/ingest-settings-section.tsxfrontend/app/settings/_components/model-provider-card.tsxfrontend/app/settings/_components/model-provider-dialog-footer.tsxfrontend/app/settings/_components/ollama-settings-dialog.tsxfrontend/app/settings/_components/openai-settings-dialog.tsxfrontend/app/settings/_components/watsonx-settings-dialog.tsxfrontend/components/chat-renderer.tsxfrontend/components/knowledge-actions-dropdown.tsxfrontend/components/knowledge-batch-actions-bar.tsxfrontend/components/knowledge-dropdown.tsxfrontend/components/knowledge-filter-panel.tsxfrontend/components/knowledge-search-input.tsxfrontend/lib/analytics.ts
lucaseduoli
approved these changes
Jun 26, 2026
ricofurtado
pushed a commit
that referenced
this pull request
Jun 26, 2026
* chore: Segment improvements (#1963) * Update static properties * and knowledge/setting actions * avoid duplicate events * coderabbit comments
ricofurtado
added a commit
that referenced
this pull request
Jul 1, 2026
* feat: add connectors permissions for Saas (#1782) * feat: add SaaS workspace connector permissions with RBAC gates Introduce admin Connectors Permission settings (cloud-only) backed by workspace_config, enforce availability on connector APIs and OAuth init, and sync the new connectors:manage:access permission at startup without an Alembic migration. OSS/dev OSS brand bypasses policy; add dev role toggle and brand-aware proxy headers for local RBAC testing. * style: ruff autofix (auto) * address CodeRabbit comments and fix backend lint unused imports * fix backend lint * feat: workspace connector permissions with aligned SaaS policy guards Add admin Connectors Permission (workspace_config-backed) with RBAC connectors:manage:access, enforce policy on connector APIs and OAuth init, and keep the permission list independent of the live connectors tab. Consolidate frontend connector-access hooks into useGetConnectorsQuery and settings tab helpers into brand.ts. Fix Connectors Permission tab redirects (RSC dev-brand default, wait for permissionsResolved). Let explicitly enabled connectors override deployment visibility filters; add OPENRAG_DEV_CONNECTOR_POLICY for local OSS dev backend enforcement Snapshot and restore all cached connector query keys in connect/disconnect mutations so optimistic updates stay correct when policy context changes mid-mutation (brand toggle, permissions resolving). * style: ruff autofix (auto) * addressed coderabbit comments * remove mentions of dev only envs in the env.example --------- Co-authored-by: Olfa Maslah <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Edwin Jose <[email protected]> * fix: google to support WEBHOOK_BASE_URL (#1849) * Update connector.py * update env in settings * fix webhook url of sharepoint * fix: make connector webhooks work end-to-end (Drive routing, Graph subscriptions, change discovery) (#1867) * fix: alias legacy /connectors/google/webhook to google_drive and reload connections store on webhook channel lookup miss * Fix Sharepoint and Google Drive * fix: propagate deleted items from SharePoint/OneDrive webhook delta query Main's delete-event coverage (#1852) expects handle_webhook to return deleted file ids so sync_specific_files can run its deleted-at-source cleanup (get_file_content -> 404 -> delete indexed chunks). The delta implementation skipped deleted items; now deleted files propagate and deleted folders stay excluded. * use logging _config * Update test_webhook_type_alias.py * fix: forward end-user JWT to MCP/API callers via gateway-placed X-OpenRAG-API-JWT header (#1874) * use fallback header * Notes on new header * raises http exception for missing jwt * feat: Add shared flag for COS ingestion to allow documents to be indexed without an owner * style: ruff autofix (auto) * Update src/api/connectors.py Improving error return Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat: Implement shared flag for document replacement in OpenSearch queries * style: ruff autofix (auto) * refactor: Remove shared flag from document ingestion options in SDK * feat: Update resolve_shared_owner_fields to return anonymous user details for shared documents * style: ruff autofix (auto) * feat: Add anonymous delete permission for shared documents and update related functionality * feat: Update delete permission to include anonymous access for shared documents * style: ruff autofix (auto) * style: apply ruff autofix * feat: Restrict shared flag usage to ibm_cos connector in sync_connector_files method * fix: clarify behavior of documents with owner=null in DLS tests * fix button types (#1875) * refactor: no derived state (#1886) * no derivied state * coderabbit comments * OPENSEARCH_NODE_COUNT_CHECK_ENABLED as deafult flag (#1975) What changed: - Renamed the env var read by the backend to OPENSEARCH_NODE_COUNT_CHECK_ENABLED in src/config/settings.py. - Updated Docker Compose to pass OPENSEARCH_NODE_COUNT_CHECK_ENABLED in docker-compose.yml. - Added the canonical flag to the example env file in .env.example. - Documented the flag in the OpenSearch config table in docs/docs/reference/configuration.mdx. - Updated the existing readiness test wording in tests/unit/test_opensearch_wait_node_count.py. - Added a regression test that loads config.settings with OPENSEARCH_NODE_COUNT_CHECK_ENABLED=false and verifies the canonical flag is what the code reads in tests/unit/config/ test_opensearch_node_count_check_enabled.py. * chore: Segment improvements (#1971) * chore: Segment improvements (#1963) * Update static properties * and knowledge/setting actions * avoid duplicate events * coderabbit comments * fix: onboarding timeout cleanup issue (#1977) * fix the cleanup timeout issue for the embedding step * final step fix * feat: add shared parameter to connector_sync function * feat: add shared parameter to TaskProcessor and update connector_sync for permission checks * fix: improve response handling in delete_document_endpoint for better status reporting * fix: clear completeTimeoutRef after completion to prevent memory leaks * fix: update OPENSEARCH_NODE_COUNT_CHECK_ENABLED to use fallback from OPENSEARCH_NODE_COUNT_CHECK * fix: onboarding timeout cleanup issue (#1977) * fix the cleanup timeout issue for the embedding step * final step fix * fix: support big csv files on langflow-less ingestion (#1968) * fix max tokens when splitting for langflow-less ingestion * fix coderabbit picks * style: ruff autofix (auto) * added non lf ingestion test --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * fix: upgrade OpenSearch to 3.6.0 (#1972) * upgrade OpenSearch to 3.6.0 * fixed PR review comments * fix nitpicks * Added ibm watsonx ai and langchain ibm to Langflow image (#1989) * OPENSEARCH_NODE_COUNT_CHECK_ENABLED as deafult flag (#1975) What changed: - Renamed the env var read by the backend to OPENSEARCH_NODE_COUNT_CHECK_ENABLED in src/config/settings.py. - Updated Docker Compose to pass OPENSEARCH_NODE_COUNT_CHECK_ENABLED in docker-compose.yml. - Added the canonical flag to the example env file in .env.example. - Documented the flag in the OpenSearch config table in docs/docs/reference/configuration.mdx. - Updated the existing readiness test wording in tests/unit/test_opensearch_wait_node_count.py. - Added a regression test that loads config.settings with OPENSEARCH_NODE_COUNT_CHECK_ENABLED=false and verifies the canonical flag is what the code reads in tests/unit/config/ test_opensearch_node_count_check_enabled.py. * fix: onboarding timeout cleanup issue (#1977) * fix the cleanup timeout issue for the embedding step * final step fix * fix: universal file names (#1982) * ascii support for ingest files * update the flow json * style: ruff autofix (auto) * Update langflow_headers.py * Update test_langflow_ingest_callback.py * Update test-ci.yml * Update test_onboarding_sample_docs.py * style: ruff autofix (auto) * Update test_ascii_safe_header_value.py * Update langflow_file_service.py * re trigger commit * fix: purge config_manager singleton in non-langflow ingestion test The _purge_modules() list was missing "config.config_manager", so the ConfigManager singleton retained its cached _config from a previous test. Subsequent calls to get_openrag_config() returned the old config where disable_ingest_with_langflow=False, causing the router to use the Langflow path even though DISABLE_INGEST_WITH_LANGFLOW=true was set in the environment. Adding config.config_manager to the purge list forces a fresh load on the next import, picking up the correct env-var values. * fix: use hash_id to derive expected document_id in CSV ingestion test DocumentFileProcessor ignores Docling's binary_hash field and computes document_id = hash_id(file_path) from the actual file content. The test was asserting against the literal mock value "sha-csv-integration-123" which never appears in the indexed documents, giving 0 hits and a false failure. Now the test derives expected_document_id via hash_id(csv_path) to match what the production code actually indexes. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --------- Co-authored-by: Wallgau <[email protected]> Co-authored-by: Olfa Maslah <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Edwin Jose <[email protected]> Co-authored-by: Edwin Jose <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Mike Fortman <[email protected]> Co-authored-by: Sebastián Estévez <[email protected]> Co-authored-by: Lucas Oliveira <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update static properties
and knowledge/setting actions
avoid duplicate events
Cherry pick of #1963
Summary by CodeRabbit