Skip to content

fix: adds safety guard to ag grid#1946

Merged
mfortman11 merged 4 commits into
mainfrom
ag-grid-saftey-guard
Jun 23, 2026
Merged

fix: adds safety guard to ag grid#1946
mfortman11 merged 4 commits into
mainfrom
ag-grid-saftey-guard

Conversation

@mfortman11

@mfortman11 mfortman11 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Problem: Calling api.forEachNode() / api.getSelectedRows() before AG Grid's row model is fully initialized causes a RowApiModule not registered error. This is a timing-dependent race condition on mount — more likely on slower machines or with React strict mode.

Fix: Added a gridReadyRef flag set by AG Grid's onGridReady callback, and a getGridApi() helper that returns null until the grid is ready. All 5 grid API call sites now go through this helper, so they safely no-op if called before initialization.

Summary by CodeRabbit

Bug Fixes

  • Improved reliability of selection and bulk-delete operations on the Knowledge page by guarding grid actions until the grid is fully initialized (and safely handling teardown).
  • Enhanced the chat “waiting too long” timeout behavior by correcting the timer trigger logic to respond reliably to streaming state.
  • Improved AG Grid runtime support by registering the required Row API module so related grid functionality is available at runtime.

@github-actions github-actions Bot added community frontend 🟨 Issues related to the UI/UX labels Jun 23, 2026
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d7ba385c-ed67-4228-b1f9-e5e7ad9c17af

📥 Commits

Reviewing files that changed from the base of the PR and between f8ee796 and b20e060.

📒 Files selected for processing (1)
  • frontend/app/knowledge/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/app/knowledge/page.tsx

Walkthrough

Three independent changes modernize grid API access and effect dependencies: gridReadyRef and getGridApi() guard AG Grid API calls in frontend/app/knowledge/page.tsx to prevent execution before initialization; both onGridReady handlers set this flag; RowApiModule is registered in frontend/components/AgGrid/registerAgGridModules.ts to provide row selection capabilities; and in frontend/app/chat/page.tsx, a derived hasStreamingMessage flag optimizes the timeout effect dependency array.

Changes

AG Grid API Readiness Guard and Module Registration

Layer / File(s) Summary
RowApiModule registration
frontend/components/AgGrid/registerAgGridModules.ts
RowApiModule is added to the ag-grid-community imports and registered via ModuleRegistry.registerModules() to provide row API features for grid selection.
gridReadyRef, lifecycle handlers, and getGridApi() accessor
frontend/app/knowledge/page.tsx
Declares gridReadyRef to track grid initialization state, adds handleGridReady and handleGridPreDestroyed lifecycle handlers, and introduces a memoized getGridApi() that returns the API only when the grid is ready.
Apply guarded API access to selection and bulk-delete flows
frontend/app/knowledge/page.tsx
Updates selection pruning effect, onSelectionChanged handler, and bulk-delete complete and cancel paths to use getGridApi() with early-return on unavailability instead of direct gridRef.current?.api access.
Wire grid lifecycle handlers to both grid instances
frontend/app/knowledge/page.tsx
Both the cloud-brand and non-cloud AgGridReact components are wired with onGridReady={handleGridReady} and onGridPreDestroyed={handleGridPreDestroyed} callbacks.

Chat Timeout Effect Dependency Optimization

Layer / File(s) Summary
hasStreamingMessage derivation and useEffect dependency update
frontend/app/chat/page.tsx
Introduces a derived hasStreamingMessage boolean flag from streamingMessage and updates the waiting-too-long timeout effect dependency array to depend on isChatStreaming and hasStreamingMessage instead of the full streamingMessage object.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

refactor

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: adds safety guard to ag grid' directly summarizes the main change: adding a safety guard mechanism to AG Grid to prevent race condition errors. It is concise, specific, and clearly communicates the primary purpose of the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ag-grid-saftey-guard

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026
@mpawlow mpawlow self-requested a review June 23, 2026 16:16
@mpawlow

mpawlow commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Functional Review 1

@mfortman11

  • ❌ Problem is still occurring for me
  • Not sure if I need to do something to explicitly reset UI state
AG Grid: error #200 "Unable to use api.forEachNode as RowApiModule is not registered.  Check if you have registered the module:\n\nimport { ModuleRegistry } from 'ag-grid-community'; \nimport { RowApiModule } from 'ag-grid-community'; \n\nModuleRegistry.registerModules([ RowApiModule ]); \n\nFor more info see: https://www.ag-grid.com/react-data-grid/modules/" "\nSee https://www.ag-grid.com/react-data-grid/errors/200?_version_=34.2.0&gridId=1&gridScoped=false&rowModelType=clientSide&isUmd=false&moduleName=RowApi&reasonOrId=api.forEachNode"
image

@mpawlow mpawlow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review 1

  • ✅ Approved / LGTM 🚀
  • See PR comments: (1a), (1b) for optional consideration (both MINOR nits)
  • See Functional Review for requested changes (might be a user error on my part)

Comment thread frontend/app/knowledge/page.tsx Outdated
Comment thread frontend/app/knowledge/page.tsx
@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026

@mpawlow mpawlow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional Review 1

  • ✅ Approved / LGTM 🚀

@github-actions github-actions Bot added the lgtm label Jun 23, 2026
@mpawlow

mpawlow commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Orthogonal Error?

  • I believe this is unrelated, but encountered this new error when trying to chat after ingesting a document
Console Error

Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
image image

@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026
@mfortman11

Copy link
Copy Markdown
Contributor Author

Orthogonal Error?

  • I believe this is unrelated, but encountered this new error when trying to chat after ingesting a document
Console Error

Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.

image image

Added a small fix for this

By coercing streamingMessage to a boolean (!!streamingMessage), the effect only re-runs when it transitions between null and non-null — not on every streaming chunk. Previously it was re-firing hundreds of times during a single response because each chunk created a new object reference.

@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026
@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{"name":"HttpError","status":500,"request":{"method":"PATCH","url":"https://api.github.com/repos/langflow-ai/openrag/issues/comments/4781099650","headers":{"accept":"application/vnd.github.v3+json","user-agent":"octokit.js/0.0.0-development octokit-core.js/7.0.6 Node.js/24","authorization":"token [REDACTED]","content-type":"application/json; charset=utf-8"},"body":{"body":"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- review_stack_entry_start -->\n\n[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/langflow-ai/openrag/pull/1946?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)\n\n<!-- review_stack_entry_end -->\n<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->\n\n> [!NOTE]\n> Currently processing new changes in this PR. This may take a few minutes, please wait...\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: Path: .coderabbit.yaml\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Pro\n> \n> **Run ID**: `d7ba385c-ed67-4228-b1f9-e5e7ad9c17af`\n> \n> </details>\n> \n> <details>\n> <summary>📥 Commits</summary>\n> \n> Reviewing files that changed from the base of the PR and between f8ee7966a1935a7026dce8197cf44649bfc1aad2 and b20e060246d1ffc1deb49209f129a4f42ff18911.\n> \n> </details>\n> \n> <details>\n> <summary>📒 Files selected for processing (1)</summary>\n> \n> * `frontend/app/knowledge/page.tsx`\n> \n> </details>\n> \n> ```ascii\n>  ________________________\n> < Code review is my jam. >\n>  ------------------------\n>   \\\n>    \\   \\\n>         \\ /\\\n>         ( )\n>       .( o ).\n> ```\n\n<!-- end of auto-generated comment: review in progress by coderabbit.ai -->\n\n<!-- walkthrough_start -->\n\n## Walkthrough\n\nThree independent changes modernize grid API access and effect dependencies: `gridReadyRef` and `getGridApi()` guard AG Grid API calls in `frontend/app/knowledge/page.tsx` to prevent execution before initialization; both `onGridReady` handlers set this flag; `RowApiModule` is registered in `frontend/components/AgGrid/registerAgGridModules.ts` to provide row selection capabilities; and in `frontend/app/chat/page.tsx`, a derived `hasStreamingMessage` flag optimizes the timeout effect dependency array.\n\n## Changes\n\n**AG Grid API Readiness Guard and Module Registration**\n\n| Layer / File(s) | Summary |\n|---|---|\n| **RowApiModule registration** <br> `frontend/components/AgGrid/registerAgGridModules.ts` | RowApiModule is added to the ag-grid-community imports and registered via ModuleRegistry.registerModules() to provide row API features for grid selection. |\n| **`gridReadyRef`, `getGridApi()` helper, and call-site updates** <br> `frontend/app/knowledge/page.tsx` | Declares `gridReadyRef` to track grid initialization state and a memoized `getGridApi()` that returns the API only when the ref is set. Updates `onSelectionChanged` to early-return via `getGridApi()`, and replaces direct `gridRef.current?.api` access in bulk-delete complete and cancel paths with `getGridApi()?.deselectAll()`. |\n| **`onGridReady` handlers set `gridReadyRef`** <br> `frontend/app/knowledge/page.tsx` | Both the cloud-brand and non-cloud AG Grid `onGridReady` callbacks are updated to set `gridReadyRef.current = true`, activating the guarded accessor. |\n\n**Chat Timeout Effect Dependency Optimization**\n\n| Layer / File(s) | Summary |\n|---|---|\n| **`hasStreamingMessage` derivation and useEffect dependency update** <br> `frontend/app/chat/page.tsx` | Introduces a derived `hasStreamingMessage` boolean flag from `streamingMessage` and updates the waiting-too-long timeout effect dependency array to depend on `isChatStreaming` and `hasStreamingMessage` instead of the full `streamingMessage` object. |\n\n## Estimated code review effort\n\n🎯 2 (Simple) | ⏱️ ~12 minutes\n\n## Suggested labels\n\n`refactor`\n\n<!-- walkthrough_end -->\n<!-- pre_merge_checks_walkthrough_start -->\n\n<details>\n<summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary>\n\n### ❌ Failed checks (1 warning)\n\n|     Check name     | Status     | Explanation                                                                          | Resolution                                                                         |\n| :----------------: | :--------- | :----------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- |\n| Docstring Coverage | ⚠️ Warning | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |\n\n<details>\n<summary>✅ Passed checks (4 passed)</summary>\n\n|         Check name         | Status   | Explanation                                                                                                                                                                 |\n| :------------------------: | :------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |\n|      Description Check     | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled.                                                                                                                 |\n|         Title check        | ✅ Passed | The title describes implementing a safety guard for AG Grid, which aligns with the PR's stated objective of addressing a race condition via a gridReadyRef guard mechanism. |\n|     Linked Issues check    | ✅ Passed | Check skipped because no linked issues were found for this pull request.                                                                                                    |\n| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request.                                                                                                    |\n\n</details>\n\n<sub>✏️ Tip: You can configure your own custom pre-merge checks in the settings.</sub>\n\n</details>\n\n<!-- pre_merge_checks_walkthrough_end -->\n<!-- finishing_touch_checkbox_start -->\n\n<details>\n<summary>✨ Finishing Touches</summary>\n\n<details>\n<summary>📝 Generate docstrings</summary>\n\n- [ ] <!-- {\"checkboxId\": \"7962f53c-55bc-4827-bfbf-6a18da830691\"} --> Create stacked PR\n- [ ] <!-- {\"checkboxId\": \"3e1879ae-f29b-4d0d-8e06-d12b7ba33d98\"} --> Commit on current branch\n\n</details>\n<details>\n<summary>🧪 Generate unit tests (beta)</summary>\n\n- [ ] <!-- {\"checkboxId\": \"f47ac10b-58cc-4372-a567-0e02b2c3d479\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Create PR with unit tests\n- [ ] <!-- {\"checkboxId\": \"6ba7b810-9dad-11d1-80b4-00c04fd430c8\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Commit unit tests in branch `ag-grid-saftey-guard`\n\n</details>\n\n</details>\n\n<!-- finishing_touch_checkbox_end -->\n<!-- tips_start -->\n\n---\n\nThanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=langflow-ai/openrag&utm_content=1946)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.\n\n<details>\n<summary>❤️ Share</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)\n- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)\n- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)\n- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)\n\n</details>\n\n\n<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>\n\n<!-- tips_end -->"},"request":{"retryCount":3,"signal":{},"retries":3,"retryAfter":16}}}

@github-actions github-actions Bot added bug 🔴 Something isn't working. and removed bug 🔴 Something isn't working. labels Jun 23, 2026
@mfortman11 mfortman11 merged commit 0569c4c into main Jun 23, 2026
16 checks passed
@github-actions github-actions Bot deleted the ag-grid-saftey-guard branch June 23, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🔴 Something isn't working. community frontend 🟨 Issues related to the UI/UX lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants