Add forced currentUser update to settings and teams pages #1535
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughInternal navigation updated to use cyberstormLink with linkId instead of hrefs, including desktop and mobile menus. Previously commented settings-related routes are re-enabled, adding /settings, /teams, and /teams/:namespaceId with nested tabs to the route configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Nav as Navigation UI
participant Router as Router
participant Page as Target Page
rect rgb(230,245,255)
note over User,Nav: Internal navigation via linkId
User->>Nav: Click "Settings" (cyberstormLink, linkId=Settings)
Nav->>Router: navigate(linkId=Settings)
Router->>Page: Resolve /settings route
Page-->>User: Render Settings view
end
rect rgb(245,235,255)
note over User,Router: Team scoped navigation
User->>Nav: Click Team "Members"
Nav->>Router: navigate(linkId=Teams, params:{namespaceId})
Router->>Page: Match /teams/:namespaceId/members
Page-->>User: Render Members tab
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (1)
547-569: Mobile user popover: inconsistent CSS classes on links (likely broken styling).
Both links userootClasses="__item --thick"whereas the rest of the mobile popover usesmobile-navigation__popover-item mobile-navigation__popover--thick. Fix to keep spacing/tap-targets consistent.Apply:
- <NewLink + <NewLink primitiveType="cyberstormLink" linkId="Settings" csVariant="primary" - rootClasses="__item --thick" + rootClasses="mobile-navigation__popover-item mobile-navigation__popover--thick" > @@ - <NewLink + <NewLink primitiveType="cyberstormLink" linkId="Teams" csVariant="primary" - rootClasses="__item --thick" + rootClasses="mobile-navigation__popover-item mobile-navigation__popover--thick" >apps/cyberstorm-remix/app/routes.ts (1)
57-63: /settings routes enabled — consider using index() helper for consistency.
Verified files exist and export default components; safe to replace the child index route with index(...).- route("/settings", "settings/user/Settings.tsx", [ - route("", "settings/user/Connections/Connections.tsx", { - index: true, - }), - route("account", "settings/user/Account/Account.tsx"), - ]), + route("/settings", "settings/user/Settings.tsx", [ + index("settings/user/Connections/Connections.tsx"), + route("account", "settings/user/Account/Account.tsx"), + ]),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx(5 hunks)apps/cyberstorm-remix/app/routes.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: chromatic-deployment (apps/cyberstorm-storybook, CHROMATIC_CYBERSTORM_TOKEN)
- GitHub Check: Generate visual diffs
- GitHub Check: CodeQL
🔇 Additional comments (3)
apps/cyberstorm-remix/app/commonComponents/Navigation/Navigation.tsx (2)
396-398: Desktop: Teams link via cyberstormLink — confirmed.
LinkId "Teams" exists in LinkLibrary (apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx and apps/cyberstorm-storybook/LinkLibrary.tsx) mapping to /teams/, and routes.ts registers route("/teams", "settings/teams/Teams.tsx"). No change required.
163-165: Confirmed — PackageUpload wired to /package/create.
PackageUpload maps to "/package/create" in apps/cyberstorm-remix/cyberstorm/utils/LinkLibrary.tsx and route("/package/create", "upload/upload.tsx") exists in apps/cyberstorm-remix/app/routes.ts (line 55).apps/cyberstorm-remix/app/routes.ts (1)
64-74: Teams routes enabled — structure LGTM; files & default exports verified.
Ensure pages self-guard for auth/visibility; optional nit: replace the empty default route with index() for clarity.- route("/teams/:namespaceId", "settings/teams/team/teamSettings.tsx", [ - route("", "settings/teams/team/tabs/Profile/Profile.tsx", { - index: true, - }), + route("/teams/:namespaceId", "settings/teams/team/teamSettings.tsx", [ + index("settings/teams/team/tabs/Profile/Profile.tsx"), route("members", "settings/teams/team/tabs/Members/Members.tsx"), route( "service-accounts", "settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx" ), route("settings", "settings/teams/team/tabs/Settings/Settings.tsx"), ]),
| primitiveType="cyberstormLink" | ||
| linkId="Settings" | ||
| rootClasses="dropdown__item navigation-header__dropdown-item" |
There was a problem hiding this comment.
🧩 Analysis chain
Desktop: Settings link via cyberstormLink — confirm auth gating and linkId mapping.
Looks consistent. Ensure unauthenticated users can’t access /settings content (page-level guard/redirect) and that Settings linkId exists in the link registry.
🏁 Script executed:
#!/bin/bash
# 1) Check linkId present
rg -n --hidden -C2 '\bSettings\b' | sed -n '1,200p'
# 2) Check settings pages implement auth guard/redirect patterns
rg -n -C3 -g 'apps/cyberstorm-remix/app/**' 'requireUser|useCurrentUser|redirect.*login|ProtectedRoute' \
| sed -n '1,200p'Length of output: 8278
Desktop: Settings linkId present — enforce page-level auth on /settings.
- Evidence: Settings linkId mapped (packages/cyberstorm/src/components/Links/LinkingProvider.tsx; apps/cyberstorm-remix/app/cyberstorm/utils/LinkLibrary.tsx) and route("/settings") exists (apps/cyberstorm-remix/app/routes.ts).
- Issue: I did not find a loader-level requireUser/redirect or ProtectedRoute for the settings routes; team settings even has a REMIX TODO about redirecting unauthenticated users, and user Settings imports NotLoggedIn (not a server-side guard).
- Action: Add server-side auth in the route loader (requireUser / redirect to login) or equivalent for these files: apps/cyberstorm-remix/app/settings/user/Settings.tsx and apps/cyberstorm-remix/app/settings/teams/team/tabs/Settings/Settings.tsx so unauthenticated users cannot access /settings content.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/settings/user/Settings.tsx and
apps/cyberstorm-remix/app/settings/teams/team/tabs/Settings/Settings.tsx, add a
server-side loader that enforces authentication (e.g., call your existing
requireUser or perform a redirect to the login route when no session/user is
present) so the /settings routes are protected; update the route exports to
include the loader and ensure any client-side NotLoggedIn UI remains only as a
fallback, not the primary guard.
Oksamies
force-pushed
the
09-16-enable_user_and_team_settings_pages
branch
from
c2597d4 to
cbe1789
Compare
Oksamies
changed the base branch from
graphite-base/1535
to
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
Oksamies
changed the title
apps/cyberstorm-remix/app/root.tsx
Outdated
| // shouldRevalidate function, cause for some reason | ||
| // the commits to localStorage are not done before | ||
| // the clientLoader is run. | ||
| sessionTools.sessionValid( |
There was a problem hiding this comment.
I feel this is bad pattern. Based on the naming I'd assume the function return a boolean. Well, it does, but the value is ignored. So why did we call it? Because the function had some side effects that are completely unknown to me until I read that function.
I think this should be split to two functions, isSessionValid that returns a boolean without having side effects, and another with a descriptive name for the side effects it has. Scratch that, looks the boolean value is never used anywhere, so we can drop the return value and just have function that updates the staleness. (Assuming this is still needed after the earlier comment is resolved.)
There was a problem hiding this comment.
I've added a commit that renames the function to something more appropriate to what it does. Kept it as one function as the return value is more or less a "utility" return value, cause it's not always needed in the logic it's called.
Oksamies
changed the base branch from
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
to
graphite-base/1535
Oksamies
force-pushed
the
09-16-enable_user_and_team_settings_pages
branch
from
cbe1789 to
e3a6579
Compare
Oksamies
force-pushed
the
graphite-base/1535
branch
from
f446eed to
a5a3db6
Compare
Oksamies
changed the base branch from
graphite-base/1535
to
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
|
ESLint and CodeQL checks are failing because Graphite created a temporary branch when I was fixing and submitting code in each PR in order. (someone has limited the maximum amount of pushes to 2 in a single command and graphite normally tries to do the whole stack at once to prevent needing to create temporary branches) |
Oksamies
changed the base branch from
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
to
graphite-base/1535
Oksamies
force-pushed
the
09-16-enable_user_and_team_settings_pages
branch
from
e3a6579 to
6ae0418
Compare
Oksamies
force-pushed
the
graphite-base/1535
branch
from
a5a3db6 to
48c96f4
Compare
Oksamies
changed the base branch from
graphite-base/1535
to
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
Oksamies
changed the base branch from
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
to
graphite-base/1535
Oksamies
force-pushed
the
graphite-base/1535
branch
from
48c96f4 to
67a0ad8
Compare
Oksamies
force-pushed
the
09-16-enable_user_and_team_settings_pages
branch
from
6ae0418 to
0fda05d
Compare
Oksamies
changed the base branch from
graphite-base/1535
to
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
| request.url.startsWith(`${publicEnvVariables.VITE_BETA_SITE_URL}/teams`) || | ||
| request.url.startsWith(`${publicEnvVariables.VITE_BETA_SITE_URL}/settings`) | ||
| ) { | ||
| forceUpdateCurrentUser = true; |
There was a problem hiding this comment.
Minor: IMO we could just as well call sessionTools.setSessionStale() here? Wouldn't require forceUpdateCurrentUser variable here or argument in getSessionCurrentUser. I.e. simpler but does the same thing (if I'm reading the code right).
| } | ||
|
|
||
| export const sessionValid = ( | ||
| export const runSessionValidationCheck = ( |
There was a problem hiding this comment.
Minor: I think this shouldn't return boolean as the value is never used for anything and removing it would simplify the implementation.
As the function has a side-effect on the localStorage values, the old name was misleading in it's effects
Oksamies
force-pushed
the
09-17-add_a_option_to_force_currentuser_update_to_getsessioncurrentuser
branch
from
67a0ad8 to
c7c481d
Compare
Oksamies
force-pushed
the
09-16-enable_user_and_team_settings_pages
branch
from
0fda05d to
1537ac3
Compare
2 participants
Summary by CodeRabbit
New Features
Refactor