Adding Update on ajax feature

[gone]

This adds a feature to allow the toolbar to watch for ajax requests and automatically update debug information to show details from the ajax request.

Attempt at solving #1575

[tim-schilling]

I like this a lot. The two things I'd advocate for are:

1. Add a debouncing/throttling to the client-side refresh logic. Yes we are debugging, but it's not a good excuse for slowing things down known web app patterns when there's a reasonable and known way to mitigate the issue. Admittedly, the recent issues about the toolbar slowing down web apps is influencing my opinion here.
2. Refactor the history panel's overload of process_request to create a new generate_headers function on Panel. This should ideally result in the removal DebugToolbarMiddleware.generate_server_timing_header because that would be in the new logic.

The more I think about how HistoryStoreForm is wrapped by SignedDataForm, the less I'm sure it's necessary. The worry that someone will manipulate the store id to access data they don't have access to is pretty far out of scope of the toolbar's responsibilities. The toolbar in its current state will render out all requests from any user that trigger the toolbar to run. If a developer needs to limit that, the configuration does support that. All that to say, I'm good with just using HistoryStoreForm.

@matthiask opinions?

[gone]

Couple of changes here:

1. Added a debounce function to the clientside logic
2. Created a generate_headers call on the toolbar and the panels - because the server timing joins across all panels into a single header I couldn't figure out a good way to move that logic in to the panels but open to ideas if you have one!
3. Refactored the test for the sidebar url to be less code
4. Replaced the bool "should_observe" with a function

I agree with your logic for the form signing, so if I don't hear anything to the contrary I'll pull it out next

[tim-schilling]

I agree with your logic for the form signing, so if I don't hear anything to the contrary I'll pull it out next

Could you open that up in a new PR if you don't mind? It'll help keep this easy to review and our discussions limited.

[gone]

PR opened - I think if that one goes through the final change I'd like to propose to this PR is adding an "include historical data" option to the history form, defaulting it to true, updating the history panels' forms to pass false and remove the two frontend delete statements. That way the old tests will continue to pass without being updated and I look into new tests for the new behavior.

[gone]

I think this is in a state ready for final review - let me know if there's any more tests/documentation that you'd like added

[matthiask]

I haven't tested the code, but it looks good to me.

Maybe a few inputs

• We mostly use the djdt prefix, this may be better than DJ-TOOLBAR-* as a header
• I think using response.headers[key] = value is a bit clearer than response[key] = value for setting headers, but that's a matter of taste
• I'd call the boolean field do_not_include_history (no abbreviations) or maybe include_history while flipping the default value of the field? (but that's bikeshedding)
• I'm unsure about the OBSERVE_REQUEST_CALLBACK name. It doesn't immediately communicate that this is about whether the toolbar should automatically update or not. Naming is hard and I don't have a better idea though.

[gone]

should_observe_ajax_callback ?

update_on_ajax_callback ?

[matthiask]

I thought about trace_request but this immediately brought me back to observe_request.

So I now think observing is fine :)

[gone]

Alright I think I got the changes in -

for include_history vs do_not_include_history - I agree that using the positive case is normally better, but if I do that then the javascript needs to include the value. I think it's simpler to let the JS not care about it because we don't have to specify in the dj template

As for the name - happy to do whatever if people have strong feelings :)

[tim-schilling]

@matthiask can you review the latest version of this and see if you have any qualms?

@gone I rearranged things after taking a longer look. Let me know if there are any issues.

[gone]

Tests pass on my end - this is much clearer. Potentially in get headers the dict could replaced with a default dict to shave off the if check, but then it would always do string concatenation and would be slower.

[tim-schilling]

I like that approach

…

On Sat, Feb 19, 2022, 11:13 AM Ben Beecher ***@***.***> wrote: Tests pass on my end - this is much clearer. Potentially in get headers the dict could replaced with a default dict to shave off the if check, but then it would always do string concatenation and would be slower. — Reply to this email directly, view it on GitHub <#1577 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJYZPY4ORZ6ULWMQ3A5VG3U37FUPANCNFSM5MDMMTIQ> . You are receiving this because you commented.Message ID: ***@***.***>

[gone]

Tried an implementation and threw it away - you still end up needing to look at the old value to know if you should include the leading comma or not so it's just as long but slower :(

[tim-schilling]

Squashed the commits and rebased on main. Cleaned up some things in history.js. Added a reference to the functionality in the change log. Should be ready to be merged.

[gone]

Beat me to fixing the test by 2 min :)

[tim-schilling]

I added a view and JS to the example app to support testing this manually more easily in the future. Really nice work @gone! Thank you.