Skip to content

Remove onetime from assertCurrentUser, pass token and user as arguments#9147

Closed
Copilot wants to merge 4 commits into
mainfrom
copilot/follow-sunset-advice
Closed

Remove onetime from assertCurrentUser, pass token and user as arguments#9147
Copilot wants to merge 4 commits into
mainfrom
copilot/follow-sunset-advice

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 2, 2026 •
edited
Loading

  • Rename assertCurrentUser → assertMatchesCurrentUser
  • Remove loggedInUser parameter; call getLoggedInUser() internally
  • Update call sites in v3uncached and v4uncached
  • Update export name

Copilot AI changed the title [WIP] Fix implementation based on Sunset's feedback Remove onetime from assertCurrentUser, pass token and user as arguments Apr 2, 2026
Copilot AI requested a review from fregante April 2, 2026 10:23
fregante
fregante reviewed Apr 2, 2026
View reviewed changes
Comment thread source/github-helpers/api.tsx Outdated
await assertCurrentUser();
const loggedInUser = getLoggedInUser();
if (personalToken && loggedInUser) {
await assertCurrentUser(personalToken, loggedInUser);
Copy link
Copy Markdown
Member

@fregante fregante Apr 2, 2026

Choose a reason for hiding this comment

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

@copilot alter the assertion function to accept undefined so the user doesn't have to manually nullcheck every time

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Done in 8975a59. assertCurrentUser now accepts string | undefined for both parameters and handles the early return internally, so callers just pass the values directly.

@fregante
Copy link
Copy Markdown
Member

fregante commented Apr 2, 2026

@SunsetTechuila is this what you had in mind?

Copilot AI requested a review from fregante April 2, 2026 10:39
Copilot AI requested a review from SunsetTechuila April 2, 2026 16:22
@SunsetTechuila
Copy link
Copy Markdown
Member

SunsetTechuila commented Apr 3, 2026 •
edited
Loading

  1. As it is right now:

    • assertCurrentUser calls getToken (value is already cached, taken from memory)
    • then it calls getLoggedInUser
    • then it calls tokenUser.get (value is already cached, taken from storage)

    In total:
    - 1 memory access
    - 1 DOM operation
    - 1 storage access

    Next time assertCurrentUser is called, onetime only accesses memory once and that's all.

  2. With this PR:

    • v3uncached/v4uncached call getLoggedInUser
    • assertCurrentUser calls tokenUser.get (value is already cached, taken from storage)

    In total:
    - 1 DOM operation
    - 1 storage access

    Each subsequent invocation is as (in)expensive as the previous one.

  3. What I was suggesting:

    • v3uncached/v4uncached call getLoggedInUser
    • assertCurrentUser calls tokenUser.get (value is already cached, taken from storage)

    In total:
    - 1 DOM operation
    - 1 storage access

    Any next invocation (until the cache is cleared) takes only 1 storage access and that's all.

@refined-github refined-github deleted a comment from Copilot AI Apr 3, 2026
SunsetTechuila
SunsetTechuila reviewed Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@SunsetTechuila SunsetTechuila left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's probably best to leave it as is, or pass the token as an argument and don't change anything else.

@fregante
Copy link
Copy Markdown
Member

fregante commented Apr 3, 2026

What I was suggesting

Just do it. I had copilot open the PR because it's pointless to keep talking about these minor details. Just open the PR or make changes here

@fregante
Copy link
Copy Markdown
Member

fregante commented Apr 4, 2026

The perfect form has already been achieved. All values are cached. DX is supreme. Case closed.

@fregante fregante closed this Apr 4, 2026
@fregante fregante deleted the copilot/follow-sunset-advice branch April 4, 2026 14:48
@SunsetTechuila SunsetTechuila mentioned this pull request Apr 28, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SunsetTechuila SunsetTechuila SunsetTechuila left review comments

@fregante fregante Awaiting requested review from fregante

Assignees

@fregante fregante

Copilot code review Copilot

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fregante @SunsetTechuila