fix: onboarding timeout cleanup issue#1977
Merged
Merged
Conversation
Contributor
WalkthroughBoth onboarding components now keep their delayed completion timeout in a ref, clear it on unmount or failure, and schedule completion through that ref so re-renders do not drop the pending callback. ChangesOnboarding completion timeout handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/onboarding/_components/onboarding-upload.tsx (1)
165-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a mount-only guard for the completion timeout The
cancelledflag is flipped by this effect’s cleanup on the rerender triggered bysetShouldCreateFilter(false)/setIsCreatingFilter(true), so thefinallytimeout path is skipped while the mutation is still in flight. Completion still happens on the laterisCreatingFilter === falsererun, but this is brittle; an unmount-only ref would make the intent explicit.🤖 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/onboarding/_components/onboarding-upload.tsx` around lines 165 - 183, The completion timeout in onboarding-upload’s effect is currently gated by the effect-local cancelled flag, which gets flipped during rerenders and makes the timeout path depend on incidental cleanup behavior. Move this guard to a mount-only ref in the onboarding-upload component so the timeout is only suppressed on unmount, and keep the existing onComplete scheduling logic in the same effect that handles refetchNudges, setShouldCreateFilter, and isCreatingFilter.
🤖 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.
Nitpick comments:
In `@frontend/app/onboarding/_components/onboarding-upload.tsx`:
- Around line 165-183: The completion timeout in onboarding-upload’s effect is
currently gated by the effect-local cancelled flag, which gets flipped during
rerenders and makes the timeout path depend on incidental cleanup behavior. Move
this guard to a mount-only ref in the onboarding-upload component so the timeout
is only suppressed on unmount, and keep the existing onComplete scheduling logic
in the same effect that handles refetchNudges, setShouldCreateFilter, and
isCreatingFilter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8061286b-c4a1-4d89-8a07-28f16f785d05
📒 Files selected for processing (2)
frontend/app/onboarding/_components/onboarding-card.tsxfrontend/app/onboarding/_components/onboarding-upload.tsx
lucaseduoli
approved these changes
Jun 26, 2026
ricofurtado
pushed a commit
that referenced
this pull request
Jun 26, 2026
* fix the cleanup timeout issue for the embedding step * final step fix
ricofurtado
pushed a commit
that referenced
this pull request
Jun 30, 2026
* fix the cleanup timeout issue for the embedding step * final step fix
ricofurtado
pushed a commit
that referenced
this pull request
Jun 30, 2026
* fix the cleanup timeout issue for the embedding step * final step fix
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.
The bug: In the task monitoring effect (line ~283), when embedding tasks complete, the code calls setCurrentStep(totalSteps) to show "Done" and setTimeout(onComplete, 1000) to advance to the next step after a delay. But currentStep is in the effect's dependency array, so changing it immediately triggers effect cleanup, which calls clearTimeout(completeTimeoutId) — killing the timeout before onComplete ever fires.
The fix: Store the timeout in a ref instead of a local variable so the effect cleanup doesn't clear it. The ref-based timeout persists across the re-render caused by setCurrentStep. Added explicit cleanup on unmount and in the error handler path.
Summary by CodeRabbit