Warn user of unsaved data output edits and optionally allow to save data before page navigation. #8916 #9521
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a stable hook Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web/pgadmin/tools/sqleditor/utils/start_running_query.py:
- Around line 165-174: The code captures an unused return value from
conn.execute_async into status, catches a broad Exception, and logs without
traceback; change the assignment to discard the unused value (use _ =
conn.execute_async(...) or drop assignment), narrow the except to specific
DB/connection exceptions (e.g., psycopg.Error, ConnectionLost or your project’s
DBError types) so unexpected exceptions propagate, and replace
self.logger.error(...) with self.logger.exception(...) to include tracebacks;
keep the existing rollback logic using is_rollback_req and still return
internal_server_error(errormsg=str(e)) for handled DB/connection errors.
🧹 Nitpick comments (3)
web/pgadmin/utils/driver/psycopg3/connection.py (1)
1089-1092: Refine exception handling in async cursor cleanup.Catching bare
Exception(line 1091) is overly broad and could mask unexpected errors during cursor cleanup. Consider catching specific psycopg exceptions or asyncio-related errors, and using proper logging instead of print statements.♻️ Proposed fix
def release_async_cursor(self): if self.__async_cursor and not self.__async_cursor.closed: try: run_coroutine_threadsafe(self.__async_cursor.close_cursor(), self._loop).result() - except Exception as e: - print("Exception==", str(e)) + except (psycopg.Error, asyncio.CancelledError) as e: + current_app.logger.warning( + f"Failed to close async cursor for connection {self.conn_id}: {e}" + )web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (2)
1133-1134: Remove commented-out code.Line 1134 contains dead code that should be removed to keep the codebase clean.
🧹 Remove dead code
eventBus.fireEvent(QUERY_TOOL_EVENTS.EXECUTION_START, rsu.current.query, {refreshData: true}); - // executionStartCallback(rsu.current.query, {refreshData: true}); } else {
1307-1310: Consider simplifying dependencies now thattriggerSaveDatais stable.Since
triggerSaveDatais wrapped withuseLatestFunc, it maintains a stable reference while always invoking the latest function. The dependencies[dataChangeStore, rows, columns]are now unnecessary and cause redundant listener re-registration on each change.♻️ Simplify effect dependencies
useEffect(()=>{ eventBus.registerListener(QUERY_TOOL_EVENTS.TRIGGER_SAVE_DATA, triggerSaveData); return ()=>eventBus.deregisterListener(QUERY_TOOL_EVENTS.TRIGGER_SAVE_DATA, triggerSaveData); -}, [dataChangeStore, rows, columns]); +}, [triggerSaveData]);Or simply
[]sincetriggerSaveDatareference is guaranteed stable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
web/pgadmin/static/js/custom_hooks.jsweb/pgadmin/static/js/helpers/ModalProvider.jsxweb/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.jsweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/tools/sqleditor/utils/start_running_query.pyweb/pgadmin/utils/driver/psycopg3/connection.pyweb/pgadmin/utils/driver/psycopg3/cursor.py
🧰 Additional context used
🧬 Code graph analysis (4)
web/pgadmin/utils/driver/psycopg3/cursor.py (1)
web/pgadmin/utils/driver/psycopg3/connection.py (1)
_execute(509-516)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.js (4)
QUERY_TOOL_EVENTS(12-87)QUERY_TOOL_EVENTS(12-87)CONNECTION_STATUS(89-95)CONNECTION_STATUS(89-95)web/pgadmin/static/js/custom_hooks.js (1)
useLatestFunc(290-295)
web/pgadmin/utils/driver/psycopg3/connection.py (2)
web/pgadmin/utils/driver/psycopg3/typecast.py (1)
register_global_typecasters(131-158)web/pgadmin/utils/driver/psycopg3/cursor.py (2)
close_cursor(210-214)close_cursor(305-309)
web/pgadmin/static/js/helpers/ModalProvider.jsx (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
modalId(847-847)web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx (1)
modalId(69-69)web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx (1)
modalId(59-59)
🪛 Ruff (0.14.10)
web/pgadmin/tools/sqleditor/utils/start_running_query.py
165-165: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
172-172: Do not catch blind exception: Exception
(BLE001)
173-173: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
web/pgadmin/utils/driver/psycopg3/connection.py
1091-1091: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: run-python-tests-pg (windows-latest, 18)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (windows-latest, 13)
- GitHub Check: run-python-tests-pg (macos-latest, 18)
- GitHub Check: run-python-tests-pg (macos-latest, 16)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (13)
🔇 Additional comments (13)
web/pgadmin/static/js/helpers/ModalProvider.jsx (1)
86-97: LGTM! Modal callback wiring refactored for consistency.The refactoring centralizes modal close handling by:
- Passing
closeModaldirectly to the cancel button (line 94)- Wiring the
onCancelClickcallback through the modal'sonClosehandler (line 96)This ensures
onCancelClickis invoked consistently across all close scenarios (cancel button, backdrop click, escape key), improving the reliability of cleanup logic.web/pgadmin/utils/driver/psycopg3/connection.py (5)
199-212: LGTM! Threaded event loop implementation enables async psycopg3 in Flask.The
_start_event_loopmethod creates a dedicated asyncio event loop in a background daemon thread, which is the correct approach for integrating async operations within Flask's synchronous request-handling model. The thread is properly named for debugging, and the alive check prevents creating duplicate loops.
287-292: LGTM! Efficient busy state detection for async connections.The
is_busymethod correctly checks the connection's internal lock state to determine if a query is executing, avoiding polling overhead. This provides a lightweight way to prevent concurrent query execution on the same connection.
855-861: LGTM! Proper async execution routing through the event loop.The refactored
__internal_blocking_executecorrectly routes async cursor execution through the dedicated event loop usingrun_coroutine_threadsafe(...).result(), making it a blocking call from the caller's perspective while allowing the async psycopg3 cursor to execute on the background loop.
1109-1153: LGTM! Proper async execution with busy check and event loop coordination.The changes correctly:
- Initialize the event loop before async operations (line 1109)
- Check if the connection is busy to prevent concurrent queries (lines 1145-1148)
- Execute the query on the dedicated event loop using
run_coroutine_threadsafe(lines 1150-1153)This ensures safe concurrent execution and proper error handling.
1548-1560: LGTM! Proper async connection cleanup with loop shutdown.The
_close_asyncmethod correctly:
- Schedules connection closure on the event loop
- Stops the loop gracefully with
call_soon_threadsafe- Joins the thread with a timeout to avoid indefinite blocking
- Cleans up references to prevent resource leaks
The 1-second timeout on thread join is reasonable for cleanup operations.
web/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.js (1)
52-52: LGTM! New event constant added for data save completion.The
SAVE_DATA_ENDevent constant follows the existing naming convention and is appropriately placed with related event definitions. This supports the broader data synchronization tracking feature mentioned in the PR objectives.web/pgadmin/utils/driver/psycopg3/cursor.py (1)
280-280: Async pattern inexecuteis intentional and properly handled by all call sites.The
executemethod returns a coroutine directly (line 280) while methods likefetchmany,fetchall,fetchone, andclose_cursorwrap async operations withasyncio.run. This is an intentional design choice:executeis called from within async functions that are managed byrun_coroutine_threadsafe()in__internal_blocking_execute()(line 859, 1153), which properly awaits the coroutine. The other methods useasyncio.run()because they bridge from sync to async code. All call sites in connection.py correctly handle the coroutine return type viaawaitwithin async contexts.web/pgadmin/static/js/custom_hooks.js (1)
290-295: LGTM! Well-implemented stable callback pattern.This hook correctly implements the "latest ref" pattern for creating stable callbacks that always invoke the most recent function reference. Minor style note: the outer parentheses around the arrow function on line 294 are unnecessary.
return useCallback((...args) => fnRef.current(...args), []);web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (4)
877-880: LGTM! Clean approach for tracking data changes.Using a ref synchronized via
useEffectprovides a reliable way to check for unsaved changes without stale closure issues in callbacks.
1093-1120: LGTM! Proper state reset before fetching new data.Adding
resetSelectionAndChanges()ensures clean slate when navigating to a different data window.
1145-1160: Save confirmation modal flow looks correct.The implementation properly coordinates save completion with page navigation via the
SAVE_DATA_ENDevent listener, ensuring data is persisted before moving to the next page.
1203-1305: LGTM! Well-structured save flow with proper event coordination.Wrapping
triggerSaveDatawithuseLatestFuncensures the callback always operates on current state values. TheSAVE_DATA_ENDevent emissions correctly signal success/failure to waiting listeners.
adityatoshniwal
force-pushed
the
warn-page
branch
3 times, most recently
from
ddb75fe to
5057c30
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Line 1086: Remove the debug print("Exception==", str(e)) in connection.py and
replace it with proper logging: obtain the module/class logger (e.g.,
logging.getLogger(__name__) or an existing self.logger) and call
logger.exception(...) or logger.error(..., exc_info=True) with a clear message
including context about the failure (use the same exception variable e) so stack
trace and error details are recorded instead of printed to stdout.
♻️ Duplicate comments (1)
web/pgadmin/tools/sqleditor/utils/start_running_query.py (1)
165-174: Static analysis issues remain unaddressed.The previous review flagged three issues that are still present:
- Line 165:
statusis captured but never used - prefix with_or discard.- Line 172: Catching bare
Exceptionis too broad - consider catching specific exceptions likepsycopg.ErrororConnectionLost.- Line 173: Use
self.logger.exception(...)instead ofself.logger.error(...)to automatically include the traceback.
🧹 Nitpick comments (4)
web/pgadmin/utils/driver/psycopg3/connection.py (2)
287-292: Misleading comment - refers to lock but code checks pgconn.is_busy().The comment on Line 290 mentions "lock is 'locked'" but the implementation calls
conn.pgconn.is_busy(), which checks if the libpq connection is processing a query. Consider updating the comment to accurately describe the behavior.📝 Suggested comment fix
def is_busy(self): """Returns True if the connection is currently executing a query.""" if self.async_ == 1 and self.conn: - # If the lock is 'locked', it means a query is running + # Check if the underlying libpq connection is busy processing a query return self.conn.pgconn.is_busy() return False
1546-1558: Consider handling thread join timeout failure.The 1-second timeout on
thread.join()is reasonable, but if the thread doesn't stop in time, references are still cleared which could lead to orphaned resources. Since the thread is a daemon, it won't block shutdown, but logging a warning on join failure could help diagnose issues.📝 Optional: Add warning on join timeout
if self._thread: self._thread.join(timeout=1) + if self._thread.is_alive(): + current_app.logger.warning( + f"Event loop thread {self._thread.name} did not stop within timeout" + ) self._thread = None self._loop = Noneweb/pgadmin/utils/driver/psycopg3/cursor.py (1)
378-388: Overly broad exception handling masks potential errors.The bare
except Exceptionsilently returnsFalsefor any failure, which could mask connection issues or other errors. Consider catching more specific exceptions or logging the error for debugging.♻️ Suggested improvement
def nextset(self): """ Synchronous bridge for nextset. """ try: return run_coroutine_threadsafe( super().nextset(), self._get_loop() ).result(timeout=10) - except Exception: + except (asyncio.TimeoutError, asyncio.CancelledError): + return False + except Exception as e: + current_app.logger.debug("nextset failed: %s", e) return Falseweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
1122-1167: Review the conditional flow and cleanup logic in FETCH_WINDOW handler.The refactored handler introduces a save-or-discard modal before page navigation, which is a good UX improvement. A few observations:
One-time listener pattern: The
truethird parameter toregisterListener(lines 1128, 1153) creates one-time listeners, which is appropriate here.Potential issue: In the cleanup function (lines 1162-1166),
deregExecEndandderegSaveDataDonemight not be defined if the user never triggers the save flow. However, the optional chaining (?.()) handles this correctly.Line 1134 comment: There's a commented-out line
// executionStartCallback(...). Consider removing dead code.Flow clarity: The
impl()function handles both the out-of-sync refresh and the direct fetch paths cleanly.🔧 Suggested cleanup
eventBus.fireEvent(QUERY_TOOL_EVENTS.EXECUTION_START, rsu.current.query, {refreshData: true}); - // executionStartCallback(rsu.current.query, {refreshData: true}); } else {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
web/pgadmin/static/js/custom_hooks.jsweb/pgadmin/static/js/helpers/ModalProvider.jsxweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.jsweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/tools/sqleditor/utils/start_running_query.pyweb/pgadmin/utils/driver/psycopg3/connection.pyweb/pgadmin/utils/driver/psycopg3/cursor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/static/js/custom_hooks.js
🧰 Additional context used
🧬 Code graph analysis (4)
web/pgadmin/static/js/helpers/ModalProvider.jsx (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
modalId(847-847)web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx (1)
modalId(69-69)web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx (1)
modalId(59-59)
web/pgadmin/tools/sqleditor/__init__.py (1)
web/pgadmin/utils/driver/psycopg3/connection.py (1)
is_busy(287-292)
web/pgadmin/tools/sqleditor/utils/start_running_query.py (1)
web/pgadmin/utils/driver/psycopg3/connection.py (2)
execute_async(1088-1169)execute_void(1171-1231)
web/pgadmin/utils/driver/psycopg3/cursor.py (1)
web/pgadmin/utils/driver/psycopg3/server_manager.py (1)
connection(201-281)
🪛 Ruff (0.14.11)
web/pgadmin/tools/sqleditor/utils/start_running_query.py
165-165: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
172-172: Do not catch blind exception: Exception
(BLE001)
173-173: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
web/pgadmin/utils/driver/psycopg3/cursor.py
284-285: Avoid specifying long messages outside the exception class
(TRY003)
387-387: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (20)
web/pgadmin/static/js/helpers/ModalProvider.jsx (1)
94-96: LGTM - Cancel flow refactored to use onClose handler.The cancel callback is now deferred to the modal's
onClosehandler, invoked when the modal unmounts viacloseModal(). SincecloseModal()is called without areasonargument (Line 94),onClosewill be triggered (Line 300-301), ensuringonCancelClickis still executed.This is a cleaner pattern that consolidates all close-related callbacks through the modal lifecycle.
web/pgadmin/utils/driver/psycopg3/connection.py (5)
22-24: LGTM - Required imports for async bridging.The imports for
Threadandrun_coroutine_threadsafesupport the new per-connection event loop pattern for bridging async psycopg3 operations.
199-212: LGTM - Clean event loop lifecycle management.The implementation correctly:
- Creates a dedicated event loop per connection for async bridging
- Uses daemon threads to avoid blocking application shutdown
- Checks thread liveness before recreating resources
- Provides descriptive thread names for debugging
390-394: LGTM - Correct async-to-sync bridging for connection establishment.The pattern of using
run_coroutine_threadsafe(...).result()correctly bridges the async psycopg3 connection to synchronous Flask request handling.
1138-1145: LGTM - Resource busy check prevents concurrent query issues.The
is_busy()check before executing prevents attempting to run multiple queries simultaneously on the same async connection, which would cause undefined behavior with psycopg3's async cursors.
1414-1419: LGTM - Consistent busy check in fetch operation.The
is_busy()check mirrors the pattern inexecute_async, preventing data fetch operations while a query is still processing.web/pgadmin/utils/driver/psycopg3/cursor.py (4)
277-286: LGTM - Clean loop retrieval from connection object.The
_get_loopmethod correctly retrieves the event loop injected by the Connection class, with appropriate error handling if the loop is missing.
288-299: LGTM - Synchronous wrapper for async execute.The implementation correctly bridges the async cursor's
executemethod to synchronous code usingrun_coroutine_threadsafewith blocking.result().
349-361: LGTM - fetchwindow correctly uses sync bridge methods.The method properly leverages the refactored
scroll()andfetchmany()synchronous bridges to implement windowed fetching.
372-376: LGTM - Consistent sync bridge pattern for cursor lifecycle.The
close_cursormethod follows the samerun_coroutine_threadsafepattern as other methods, ensuring proper async cleanup.web/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.js (1)
52-52: LGTM - New event constant for save data lifecycle.The
SAVE_DATA_ENDconstant supports the unsaved data warning feature, allowing components to signal save completion status.web/pgadmin/tools/sqleditor/__init__.py (1)
1084-1088: LGTM - Enhanced busy detection using connection state.The addition of
conn.is_busy()alongside the thread alive check provides more robust detection of ongoing query execution. This catches cases where the connection is busy even if the thread state is ambiguous.web/pgadmin/tools/sqleditor/utils/start_running_query.py (2)
57-57: LGTM: Status initialization updated correctly.The change from
-1toTruealigns with the boolean status returned byexecute_async, which returns(True, None)on success and(False, errmsg)on failure according to the relevant code snippets.
184-185: LGTM: Formatting adjustment.Minor formatting change with no functional impact.
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (6)
34-34: LGTM: Import addition for stable callback hook.The
useLatestFunchook import supports the new pattern fortriggerSaveDatato avoid stale closure issues in async event handlers.
850-850: LGTM: Ref-based state tracking for async operations.The
pageDataOutOfSyncandisDataChangedRefrefs provide stable references for checking state in async callbacks and event handlers without stale closure issues. This pattern is appropriate for:
pageDataOutOfSync: Tracks when saved changes haven't been re-fetched from the serverisDataChangedRef: Provides synchronous access to whether data has unsaved changesAlso applies to: 877-880
1203-1305: LGTM: Well-structured save flow with proper event signaling.The
triggerSaveDatafunction is well-refactored:
useLatestFuncwrapper (line 1203, 1305): Ensures the callback always captures the latestdataChangeStore,rows, andcolumnsvalues, preventing stale closure bugs.
SAVE_DATA_ENDsignaling: All exit paths correctly fire the event:
- Line 1205: No changes →
SAVE_DATA_END(false)- Line 1252: Save failed (status error) →
SAVE_DATA_END(false)- Line 1292: Save succeeded →
SAVE_DATA_END(true)- Line 1303: API error →
SAVE_DATA_END(false)
pageDataOutOfSynctracking: Correctly set totrueonly on successful save (line 1256), indicating the displayed data may differ from the database until refresh.
1173-1192: LGTM: Consistent ref usage for unsaved changes check.The change at line 1175 to use
isDataChangedRef.currentis consistent with the overall refactoring pattern and avoids potential stale closure issues.
1093-1120: LGTM: Selection and changes reset on window fetch.Adding
resetSelectionAndChanges()at line 1098 ensures that when fetching a new data window, any tracked changes and selections are cleared. This is appropriate since the fetched data represents a fresh view from the server.
953-964: LGTM: Ref-based unsaved changes check.Using
isDataChangedRef.currentinstead of a computed check ensures the latest state is used in this async callback context.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
adityatoshniwal
force-pushed
the
warn-page
branch
3 times, most recently
from
e51dbd3 to
4a001e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/en_US/query_tool_toolbar.rst`:
- Around line 244-245: Update the table entry for "Show Entire Range" so the
description is clearer and ends with a period; replace "Click to get all the
rows and set from and to rows range" with a concise sentence like "Click to load
all rows and set the from/to row range." ensuring the trailing period is present
for consistency with other entries.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (2)
1125-1151: Remove commented-out code and verify listener cleanup.
- Line 1133 contains commented-out code that should be removed.
- There's a potential issue with listener cleanup: if
pageDataOutOfSync.currentis true and the component unmounts beforeEXECUTION_ENDfires,deregExecEndwill be properly cleaned up. However, if the user dismisses the confirmation dialog (lines 1145-1147),deregExecEndwon't be set but will be safely handled by optional chaining.Suggested fix
const impl = ()=> { if(pageDataOutOfSync.current) { deregExecEnd = eventBus.registerListener(QUERY_TOOL_EVENTS.EXECUTION_END, (success)=>{ if(!success) return; pageDataOutOfSync.current = false; fetchWindow(...args); }, true); eventBus.fireEvent(QUERY_TOOL_EVENTS.EXECUTION_START, rsu.current.query, {refreshData: true}); - // executionStartCallback(rsu.current.query, {refreshData: true}); } else { pageDataOutOfSync.current = false; fetchWindow(...args); } };
1293-1296: Consider simplifying the dependency array.Since
triggerSaveDatais now wrapped withuseLatestFunc, it returns a stable callback that always accesses the latest state. The dependency array[dataChangeStore, rows, columns]causes unnecessary listener re-registrations on every state change.Suggested optimization
useEffect(()=>{ eventBus.registerListener(QUERY_TOOL_EVENTS.TRIGGER_SAVE_DATA, triggerSaveData); return ()=>eventBus.deregisterListener(QUERY_TOOL_EVENTS.TRIGGER_SAVE_DATA, triggerSaveData); - }, [dataChangeStore, rows, columns]); + }, [triggerSaveData]);Since
useLatestFuncreturns a stable reference, this would register the listener only once.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/query_data_pagination.pngis excluded by!**/*.png
📒 Files selected for processing (5)
docs/en_US/query_tool_toolbar.rstweb/pgadmin/static/js/custom_hooks.jsweb/pgadmin/static/js/helpers/ModalProvider.jsxweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/utils/driver/psycopg3/server_manager.py
✅ Files skipped from review due to trivial changes (1)
- web/pgadmin/utils/driver/psycopg3/server_manager.py
🚧 Files skipped from review as they are similar to previous changes (2)
- web/pgadmin/static/js/custom_hooks.js
- web/pgadmin/static/js/helpers/ModalProvider.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
web/pgadmin/static/js/custom_hooks.js (1)
useLatestFunc(290-295)
🔇 Additional comments (7)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (7)
34-34: LGTM!The import of
useLatestFuncis correctly added to support the stable callback pattern fortriggerSaveData.
850-850: LGTM!Using
useRefforpageDataOutOfSyncis appropriate since this flag is used for flow control rather than rendering, avoiding unnecessary re-renders.
877-880: LGTM!The
isDataChangedRefpattern effectively avoids stale closure issues in async callbacks by synchronizing the ref withdataChangeStorechanges. This ensures that checks for unsaved changes always reflect the current state.
953-964: LGTM!The condition correctly uses
isDataChangedRef.currentto check for unsaved changes and skips the prompt during data refresh operations.
1098-1098: LGTM!Calling
resetSelectionAndChanges()after fetching new window data correctly clears stale selections and change tracking.
1163-1168: LGTM!The check correctly uses
isDataChangedRef.currentto determine if there are unsaved changes before showing the save prompt.
1193-1291: LGTM!The
useLatestFuncwrapper correctly ensures the callback always references the latest state, avoiding stale closure issues. ThepageDataOutOfSyncflag logic is sound:
- Set to
trueafter successful save → triggers data refresh on next page navigation (server may have applied defaults/triggers)- Set to
falseon error → no refresh needed since data wasn't modified
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
adityatoshniwal
force-pushed
the
warn-page
branch
from
4a001e3 to
7469ced
Compare
1 participant
Summary by CodeRabbit
Documentation
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.