Skip to content

Ensure worspaces/pgadmin state is saved on abrupt shutdown of pgadmin.#GH_3319 #8671

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ensure worspaces/pgadmin state is saved on abrupt shutdown of pgadmin.#GH_3319 #8671

wants to merge 2 commits into from

Conversation

yogeshmahajan-1903
Copy link
Contributor

No description provided.

@yogeshmahajan-1903 yogeshmahajan-1903 force-pushed the GH_3319 branch 5 times, most recently from ae9bd5f to 7441c9e Compare April 22, 2025 09:58
@yogeshmahajan-1903 yogeshmahajan-1903 force-pushed the GH_3319 branch 6 times, most recently from 72d6539 to c2980a1 Compare May 2, 2025 06:31
akshay-joshi
akshay-joshi requested changes May 6, 2025
View reviewed changes
Copy link
Contributor

@akshay-joshi akshay-joshi left a comment

Choose a reason for hiding this comment

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

GUI Review Comments:

  1. When opening pgAdmin again in the browser Default Workspace should be in focus. Open Schema Diff tool and refresh the browser.

  2. PSQL tools database name not showing in the single line on reopening pgAdmin.

  3. Delete all the states from the table if the user changes the preferences and sets them to False.

  4. Getting error AttributeError: 'NoneType' object has no attribute 'connection' in line 604, in _get_database_role psql/init.py

  5. params['db'] = underscore_escape(data['db_name'])  TypeError: 'NoneType' object is not subscriptable in line 104 in psql/init.py

  6. Open Query Tool/PSQL in the respective workspace and then refresh the browser. It opens in the Default workspace, which seems to be correct, but we need to document this behaviour.

  7. Query Tool/PSQL for adHoc servers are still opening with error. We should not store state of the adHoc server.

  8. The show_query_tool.js, show_erd_tool.js, showSchemaDiffTool.js and show_psql_tool.js files have duplicate code to open the respective tools. We can easily make some part of the logic generic.

  9. Why file name 'showSchemaDiffTool.js' different in respect to others like show_query_tool.js

  10. Documentation/Screenshot updates are missing for Preferences.

  11. Create a new .rst file to explain this new feature.

  12. Add comments in the new function added in this PR and wherever needed in the code, so that others can understand the logic.

adityatoshniwal
adityatoshniwal requested changes May 8, 2025
View reviewed changes
Copy link
Contributor

@adityatoshniwal adityatoshniwal left a comment

Choose a reason for hiding this comment

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

Have we handled the case where a server is removed?

web/pgadmin/browser/static/js/browser.js Outdated
if(res.data.success && res.data.data.result.length > 0){
//let oldIds = []
_.each(res.data.data.result, function(tool_data){
if (tool_data.tool_name == 'sqleditor'){
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather use events to trigger open. This loop will be synchronous, events will be async which will improve performance.

web/pgadmin/settings/__init__.py
data = json.loads(request.data)
id = data['trans_id']
fernet = Fernet(current_app.config['SECRET_KEY'].encode())
tool_data = fernet.encrypt(json.dumps(data['tool_data']).encode())
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure API Content-Type is gzip on POST and GET both for low network footprint.

web/pgadmin/settings/__init__.py
'tool_data': fernet.decrypt(row.tool_data).decode(),
'id': row.id
})
return make_json_response(
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure API response Type is gzip on POST and GET both for low network footprint.

web/pgadmin/static/js/PgAdminProvider.jsx Outdated
const eventBus = React.useRef(new EventBus());

React.useEffect(()=>{
eventBus.current.registerListener('SAVE_TOOL_DATA', (data) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not the right place for putting any API call. PgAdmin provider should be purely used for pgAdmin instance only. It can be used at places where there is no need to save tool data.

web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx Outdated
@@ -156,6 +156,21 @@ export default class ERDTool extends React.Component {
this.forceClose = this.closePanel;
}

saveERDToolData = () => {
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a timeout?

web/pgadmin/tools/erd/static/js/show_erd_tool.js Outdated


// Callback to draw ERD Tool for objects
export function relaunchErdTool(state_info) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets not duplicate launch. Follow flux architecture.

web/pgadmin/tools/psql/static/js/components/PsqlComponent.jsx
setTheme();

termRef.current.focus();
const observer = new ResizeObserver((entries) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should create a react hook for ResizeObserver now. It is present at many places.

web/pgadmin/tools/schema_diff/__init__.py Outdated
@@ -121,7 +120,7 @@ def index():

@blueprint.route(
'/panel/<int:trans_id>/<path:editor_title>',
methods=["GET"],
methods=["POST"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are your making it as POST? This will create problem when refreshing the schema diff in a new tab.

web/pgadmin/tools/schema_diff/static/js/components/SchemaDiffCompare.jsx Outdated
@@ -641,7 +657,16 @@ export function SchemaDiffCompare({ params }) {
} else {
setTargetDatabaseList(res.data.data);
}

if(params.params.tool_data){
let data = JSON.parse(params.params.tool_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

this code looks repeated.

web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx Outdated
const change = useCallback(()=>{
eventBus.fireEvent(QUERY_TOOL_EVENTS.QUERY_CHANGED, editor.current.isDirty());

const debouncedSave = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use lodash debounce? We also have a hook useDelayDebounce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshay-joshi akshay-joshi akshay-joshi requested changes

@adityatoshniwal adityatoshniwal adityatoshniwal requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yogeshmahajan-1903 @akshay-joshi @adityatoshniwal