Skip to content

Fix all Copilot review issues - #6

Merged
PriyanshAg-1 merged 10 commits into
mainfrom
fix-copilot-reviews
Apr 22, 2026
Merged

Fix all Copilot review issues#6
PriyanshAg-1 merged 10 commits into
mainfrom
fix-copilot-reviews

Conversation

@capy-ai

@capy-ai capy-ai Bot commented Apr 22, 2026

Copy link
Copy Markdown

Fixes all Copilot AI review comments from PR OpenCloudGaming#344

…rror recovery state sync, ARIA semantics, SavedAccount type usage

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses prior automated review feedback by tightening types in the main-process auth service and adjusting renderer navbar accessibility attributes for the account switcher UI.

Changes:

  • Update AuthService.getSavedAccounts() to return the shared SavedAccount type.
  • Adjust ARIA attributes/roles for the navbar account dropdown trigger and popup.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
opennow-stable/src/renderer/src/components/Navbar.tsx Updates ARIA attributes/roles for the account dropdown trigger and container.
opennow-stable/src/main/gfn/auth.ts Uses shared SavedAccount typing for the saved accounts API surface.

367 className="navbar-user navbar-user--clickable"
368 onClick={() => setAccountDropdownOpen((previous) => !previous)}
369 aria-expanded={accountDropdownOpen}
370 aria-haspopup="listbox"
Comment on lines +391 to +392
391 <div className="navbar-account-dropdown" role="list" aria-label="Switch account">
392 <div className="navbar-account-dropdown-header">Switch Account</div>

@capy-ai capy-ai Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added 3 comments

@PriyanshAg-1

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

Repository owner deleted a comment from capy-ai Bot Apr 22, 2026
Repository owner deleted a comment from capy-ai Bot Apr 22, 2026
Repository owner deleted a comment from capy-ai Bot Apr 22, 2026
@PriyanshAg-1
PriyanshAg-1 requested a review from Copilot April 22, 2026 18:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR follows up on PR OpenCloudGaming#344 by addressing prior Copilot review feedback across the renderer navbar and main-process auth service.

Changes:

  • Adjusts the Navbar account-switch dropdown ARIA attributes/roles.
  • Refactors AuthService.getSavedAccounts() to return the shared SavedAccount[] contract type.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
opennow-stable/src/renderer/src/components/Navbar.tsx Updates accessibility attributes/roles for the account dropdown UI.
opennow-stable/src/main/gfn/auth.ts Aligns saved-account typing with shared contract and tweaks session refresh/expiry result handling.

Comment on lines +365 to +392
365 <button
366 type="button"
367 className="navbar-user navbar-user--clickable"
368 onClick={() => setAccountDropdownOpen((previous) => !previous)}
369 aria-expanded={accountDropdownOpen}
370 aria-haspopup="listbox"
371 >
372 {user.avatarUrl ? (
373 <img src={user.avatarUrl} alt={user.displayName} className="navbar-avatar" />
374 ) : (
375 <div className="navbar-avatar-fallback">
376 <User size={14} />
377 </div>
378 )}
379 <div className="navbar-user-info">
380 <span className="navbar-username">{user.displayName}</span>
381 {tierInfo && (
382 <span className={`navbar-tier ${tierInfo.className}`}>{tierInfo.label}</span>
383 )}
384 </div>
385 <ChevronDown
386 size={14}
387 className={`navbar-user-chevron${accountDropdownOpen ? " is-open" : ""}`}
388 />
389 </button>
390 {accountDropdownOpen && (
391 <div className="navbar-account-dropdown" role="list" aria-label="Switch account">
392 <div className="navbar-account-dropdown-header">Switch Account</div>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Already addressed in commit 9eae745. The account dropdown now uses consistent menu semantics: aria-haspopup="menu" on the trigger, role="menu" on the container, and role="menuitem" on all four interactive buttons (account switch, remove, add account, sign out all).

Comment thread opennow-stable/src/main/gfn/auth.ts Outdated
Comment on lines +1102 to +1113
1102 if (!tokens.clientToken && !tokens.refreshToken) {
1103 if (expired) {
1104 await this.logout();
1105 return {
1106 session: null,
1107 refresh: {
1108 attempted: true,
1109 forced: forceRefresh,
1110 outcome: "missing_refresh_token",
1111 message: "Saved session expired and has no refresh mechanism. Please log in again.",
1112 },
1113 };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 9eae745. After await this.logout(), session: null is now replaced with session: this.getSession() in the !tokens.clientToken && !tokens.refreshToken expired branch. The logout() method updates all in-memory state synchronously (deletes current session, advances activeUserId to next saved account) before its async persist() call, so getSession() correctly returns the new active session after the await.

Comment thread opennow-stable/src/main/gfn/auth.ts Outdated
@PriyanshAg-1

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

2 similar comments
@PriyanshAg-1

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

@PriyanshAg-1

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…after logout

Agent-Logs-Url: https://github.com/priyanshrv1-oss/OpenNOW1/sessions/bc9cb33e-8e97-4cf8-8986-2a9a584abddc

Co-authored-by: PriyanshAg-1 <204138848+PriyanshAg-1@users.noreply.github.com>

Copilot AI commented Apr 22, 2026

Copy link
Copy Markdown

@copilot apply changes based on the comments in this thread

Fixed in commit 9eae745. The account dropdown now uses consistent menu semantics throughout: aria-haspopup="menu" on the trigger button, role="menu" on the dropdown container, and role="menuitem" on all interactive buttons inside (account switch, remove, add account, sign out all).

@copilot apply changes based on the comments in this thread

Fixed in commit 9eae745 — see the reply to the inline comment thread for details on the ARIA menu role changes and the auth session fix.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses prior Copilot review feedback from PR OpenCloudGaming#344 by tightening multi-account/session behavior in the main auth service and improving ARIA semantics for the renderer account switch dropdown.

Changes:

  • Renderer: updates the account switch dropdown to use menu semantics (aria-haspopup="menu", role="menu", role="menuitem").
  • Main: updates AuthService.getSavedAccounts() to return the shared SavedAccount[] type and keeps session state consistent in refresh/expiry flows.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
opennow-stable/src/renderer/src/components/Navbar.tsx Adjusts account dropdown ARIA roles/attributes and related UI structure.
opennow-stable/src/main/gfn/auth.ts Aligns saved-account typing with shared contract and refines session refresh/expiry handling.

Comment on lines +391 to +393
391 <div className="navbar-account-dropdown" role="menu" aria-label="Switch account">
392 <div className="navbar-account-dropdown-header">Switch Account</div>
393 <div className="navbar-account-list">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit ffab5bb. Added role="none" to the three non-interactive structural wrapper elements inside role="menu": the header div (.navbar-account-dropdown-header), the list wrapper (.navbar-account-list), and each account row wrapper (.navbar-account-item). The divider already had role="separator". All interactive buttons retain role="menuitem", so the ARIA owned-element contract for role="menu" is now valid.

Comment thread opennow-stable/src/renderer/src/components/Navbar.tsx Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ne numbers in TS files

Agent-Logs-Url: https://github.com/priyanshrv1-oss/OpenNOW1/sessions/f707cd73-f431-41cf-96a8-c5e0c34ab939

Co-authored-by: PriyanshAg-1 <204138848+PriyanshAg-1@users.noreply.github.com>

Copilot AI commented Apr 22, 2026

Copy link
Copy Markdown

@copilot apply changes based on the comments in this thread

Already addressed in commit 9eae745 (aria-haspopup="menu", role="menu" on container, role="menuitem" on buttons). The structural wrapper fix (comment 3126368408) was then applied in ffab5bb.

@copilot apply changes based on the comments in this thread

Already addressed — ARIA menu roles fixed in 9eae745, structural wrappers (role="none") and role="separator" divider fixed in ffab5bb.

@copilot apply changes based on the comments in this thread

Already addressed — ARIA menu roles fixed in 9eae745, structural wrappers (role="none") and role="separator" divider fixed in ffab5bb.

@copilot apply changes based on the comments in this thread

Already addressed — ARIA menu roles fixed in 9eae745, structural wrappers (role="none") and role="separator" divider fixed in ffab5bb.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the account-switch dropdown accessibility semantics in the renderer and aligns the main-process AuthService.getSavedAccounts() return type with the shared cross-process contract in @shared/gfn.

Changes:

  • Adjusts the Navbar account dropdown ARIA roles (presentational wrappers, separator, menuitems).
  • Replaces an inline getSavedAccounts() return type with the shared SavedAccount interface.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
opennow-stable/src/renderer/src/components/Navbar.tsx Refines ARIA roles within the account dropdown to better match menu semantics.
opennow-stable/src/main/gfn/auth.ts Uses SavedAccount[] from @shared/gfn for getSavedAccounts() to keep the shared contract consistent.

Comment thread opennow-stable/src/renderer/src/components/Navbar.tsx Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to address prior Copilot review feedback from PR OpenCloudGaming#344 by adjusting the navbar account dropdown’s accessibility markup and aligning the main-process getSavedAccounts() return type with the shared SavedAccount contract.

Changes:

  • Updated the account dropdown trigger/popup attributes in Navbar.tsx (ids/aria-controls, roles, divider semantics).
  • Replaced an inline structural type in AuthService.getSavedAccounts() with the shared SavedAccount interface.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

File Description
opennow-stable/src/renderer/src/components/Navbar.tsx Tweaks ARIA relationships/roles for the account dropdown UI.
opennow-stable/src/main/gfn/auth.ts Uses SavedAccount from @shared/gfn for getSavedAccounts() typing.

Comment thread opennow-stable/src/renderer/src/components/Navbar.tsx Outdated
})}
</div>
<div className="navbar-account-divider" />
<div className="navbar-account-divider" aria-hidden="true" />
Comment thread opennow-stable/src/renderer/src/components/Navbar.tsx
Comment thread opennow-stable/src/renderer/src/components/Navbar.tsx Outdated
PriyanshAg-1 and others added 2 commits April 23, 2026 01:26
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@PriyanshAg-1

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

PriyanshAg-1 and others added 2 commits April 23, 2026 01:29
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@PriyanshAg-1
PriyanshAg-1 self-requested a review April 22, 2026 20:11
@PriyanshAg-1
PriyanshAg-1 merged commit 75c26e9 into main Apr 22, 2026
4 of 9 checks passed
@capy-ai
capy-ai Bot removed the request for review from Copilot April 22, 2026 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants