-
Notifications
You must be signed in to change notification settings - Fork 809
Add a new button in the query tool data output toolbar to get entire … #9454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new button in the query tool data output toolbar to get entire … #9454
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a "Show entire range" control to the query tool results toolbar and updates pagination to emit FETCH_WINDOW(from,to); adjusts a CodeMirror test helper to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/en_US/release_notes_9_12.rst(1 hunks)web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx(3 hunks)web/regression/feature_utils/pgadmin_page.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx (2)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
eventBus(831-831)web/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.js (2)
QUERY_TOOL_EVENTS(12-86)QUERY_TOOL_EVENTS(12-86)
⏰ 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, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (windows-latest, 14)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-python-tests-pg (windows-latest, 13)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (13)
🔇 Additional comments (4)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx (3)
208-249: LGTM! Well-implemented "Show entire range" feature.The new UI controls for showing the entire data range are well-structured:
- Properly gated behind
!serverCursorcondition- The "Show entire range" button correctly updates inputs and triggers the fetch
- Appropriate disabled state when already showing the full range
- Maintains consistency with existing pagination controls
27-27: LGTM! Icon import added correctly.The
AllInboxRoundedIconimport is properly added and used in the new "Show entire range" button.
121-121: [Rewritten review comment]
[Classification tag]web/regression/feature_utils/pgadmin_page.py (1)
965-967: Verify if cmTile.view is the correct property path for accessing CodeMirror.The change from
arguments[0].cmViewtoarguments[0].cmTile.viewneeds verification to ensure it matches the actual CodeMirror initialization in the application. Confirm that:
- The CodeMirror instance is properly initialized with a
cmTileproperty containing aviewobject- The
setValue()andsetCursor()methods exist on this property- No other CodeMirror API changes have broken this test utility
343fcfe to
7c5b16f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/en_US/release_notes_9_12.rst(1 hunks)web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx(3 hunks)web/pgadmin/translations/it/LC_MESSAGES/messages.po(0 hunks)web/regression/feature_utils/pgadmin_page.py(1 hunks)
💤 Files with no reviewable changes (1)
- web/pgadmin/translations/it/LC_MESSAGES/messages.po
🚧 Files skipped from review as they are similar to previous changes (2)
- web/regression/feature_utils/pgadmin_page.py
- docs/en_US/release_notes_9_12.rst
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx (2)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
eventBus(831-831)web/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.js (2)
QUERY_TOOL_EVENTS(12-86)QUERY_TOOL_EVENTS(12-86)
⏰ 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). (6)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (13)
🔇 Additional comments (2)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx (2)
27-27: Icon import looks fine; please ensure it’s included in translations/build footprint expectations.
No functional concerns with addingAllInboxRoundedIconhere.
118-123: FETCH_WINDOW event signature mismatch: callback parameter never supplied.The
fireEvent(QUERY_TOOL_EVENTS.FETCH_WINDOW, from, to)calls (lines 121, 132, 212, 229) emit only 2 arguments, butfetchWindow()in ResultSet.jsx expects 3 parameters:(fromRownum, toRownum, callback). The callback is invoked withcallback?.()but will always be undefined since no emit site supplies it. All fireEvent calls are internally consistent with each other, but this signature mismatch indicates the callback functionality was lost when this feature was added (commit 7c5b16f). Verify whether the callback parameter should be removed fromfetchWindow()or whether callbacks need to be re-supplied by emit sites.
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSetToolbar.jsx
Show resolved
Hide resolved
7c5b16f to
dd818dc
Compare
…range of data. #8890
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.