Skip to content

cache login local storage #6839

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

cache login local storage #6839

wants to merge 9 commits into from

Conversation

wdvr
Copy link
Contributor

@wdvr wdvr commented Jun 26, 2025

• auth improvements:
– localStorage-backed 30-day cache for the “does the current user have write access to pytorch/pytorch?” check.
– new /api/torchagent-check-permissions endpoint and client-side logic to call it.
– better unauthenticated / insufficient-permission screens and a “Try again” flow.

Copy link

vercel bot commented Jun 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
torchci ✅ Ready (Inspect) Visit Preview Jul 2, 2025 9:58pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 26, 2025
Base automatically changed from wdvr/torchagent_release to main June 26, 2025 11:14
Copy link
Contributor

@izaitsevfb izaitsevfb left a comment

Choose a reason for hiding this comment

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

looks like some of these changes (e.g. NavBar) are already in the main?

please rebase and resolve conflicts

}

// Clear expired or mismatched cache
localStorage.removeItem(PERMISSION_CACHE_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: code duplication

}
};

const clearCachedPermissionState = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

Comment on lines +297 to +305
let newState: "sufficient" | "insufficient";
if (response.status === 403) {
newState = "insufficient";
} else if (!response.ok) {
// For 500 errors or other issues, also show insufficient permissions
newState = "insufficient";
} else {
newState = "sufficient";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let newState: "sufficient" | "insufficient";
if (response.status === 403) {
newState = "insufficient";
} else if (!response.ok) {
// For 500 errors or other issues, also show insufficient permissions
newState = "insufficient";
} else {
newState = "sufficient";
}
const newState = response.status === 403 || !response.ok ? "insufficient" : "sufficient";

Copy link
Contributor

Choose a reason for hiding this comment

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

probably could be simplified further to just checking response.ok

Copy link
Contributor

@izaitsevfb izaitsevfb left a comment

Choose a reason for hiding this comment

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

stamping to unblock, but I think some more cleanup is warranted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants