Skip to content

debug: recreate change in 12544 #13000

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

Closed
wants to merge 1 commit into from
+16 −5
Closed

debug: recreate change in 12544 #13000

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions appengine/standard/iap/js/poll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright Google Inc.
//
// Copyright 2017 Google LLC
Copy link

@ghost ghost Dec 15, 2024

Choose a reason for hiding this comment

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

It's standard practice to omit the year in copyright notices unless the copyright has changed hands or there's a specific legal reason to include it. Consider simplifying to // Copyright Google LLC to adhere to common conventions.

Suggested change
// Copyright 2017 Google LLC
// Copyright Google LLC


// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//

// http://www.apache.org/licenses/LICENSE-2.0
//

// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
Expand Down Expand Up @@ -54,10 +54,21 @@ function sessionRefreshClicked() {

function checkSessionRefresh() {
if (iapSessionRefreshWindow != null && !iapSessionRefreshWindow.closed) {
fetch('/favicon.ico').then(function(response) {
// Attempting to start a new session.
// XMLHttpRequests is used by the server to identify AJAX requests
fetch('/favicon.ico', {
method: "GET",
credentials: 'include',
headers: {
'X-Requested-With': 'XMLHttpRequest'
}
Comment on lines +57 to +64
Copy link

@ghost ghost Dec 15, 2024

Choose a reason for hiding this comment

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

low

While the added headers are helpful, the code lacks clarity on why they are necessary for AJAX request identification. Explain the purpose of X-Requested-With and how the server uses it. Mentioning the specific server-side framework or library that relies on this header would be very helpful. This adds valuable context for future maintainers and improves the overall understanding of the code.

}).then(function(response) {
// Checking if browser has a session for the requested app
if (response.status === 401) {
// No new session detected. Try to get a session again
window.setTimeout(checkSessionRefresh, 500);
} else {
// Session retrieved.
iapSessionRefreshWindow.close();
iapSessionRefreshWindow = null;
Comment on lines 67 to 73
Copy link

@ghost ghost Dec 15, 2024

Choose a reason for hiding this comment

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

low

The comments // Checking if browser has a session... and // Session retrieved could be more descriptive. Instead of simply stating what the code does, explain the significance of the check and the retrieval. For example, what does a 401 status indicate in this context? What happens after the session is retrieved? This will make the code easier to understand and maintain.

}
Expand Down
Loading