Logout Everywhere #8850
Logout Everywhere #8850
Conversation
📝 WalkthroughWalkthroughAdds a logout-everywhere feature: new controller endpoint and route, DAO and model additions with DB migration, token deletion and cookie invalidation logic, frontend API/UI to trigger it, routing updates, centralized request error handling, and a minor ShortLink action auth change. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/javascripts/router/router.tsx (1)
378-378: Fix broken backward-compat redirect for change passwordThe target “/account/password” no longer exists (renamed to “/account/security”). This redirect currently produces a 404.
Apply this diff:
- <Route path="/auth/changePassword" element={<Navigate to="/account/password" />} /> + <Route path="/auth/changePassword" element={<Navigate to="/account/security" />} />frontend/javascripts/admin/account/account_security_view.tsx (1)
25-37: Chain mixes then/await; error message conflates failures.If logoutUserEverywhere fails, the catch labels it “Password change failed,” which is misleading. Rewrite onFinish with async/await and separate handling. Also, rely on centralized Request toasts; avoid double error toasts here.
- function onFinish(formValues: Record<string, any>) { - changePassword(formValues) - .then(async () => { - Toast.success(messages["auth.reset_pw_confirmation"]); - await logoutUserEverywhere(); - Store.dispatch(logoutUserAction()); - navigate("/auth/login"); - }) - .catch((error) => { - console.error("Password change failed:", error); - Toast.error("Failed to change password. Please try again."); - }); - } + async function onFinish(formValues: Record<string, any>) { + try { + await changePassword(formValues); + Toast.success(messages["auth.reset_pw_confirmation"]); + try { + await logoutUserEverywhere(); + } catch (e) { + // Centralized Request handler already showed a toast; keep UX simple + console.warn("Logout-everywhere after password change failed:", e); + } + Store.dispatch(logoutUserAction()); + navigate("/auth/login"); + } catch (error) { + console.error("Password change failed:", error); + // Centralized Request handler will have shown a toast; optionally keep a generic one: + Toast.error("Failed to change password. Please try again."); + } + }
🧹 Nitpick comments (18)
app/models/user/UserService.scala (1)
220-221: PublicremoveUserFromCacheis fine; tighten predicate and consider a batched variant for “logout everywhere.”
- Visibility widening looks intentional for the new flow. Minor readability: match on the tuple to avoid magic indices.
- For performance, when invalidating many user IDs (multi-user), provide a single-shot batch clear to avoid multiple cache scans.
Apply this small readability diff:
def removeUserFromCache(userId: ObjectId): Unit = - userCache.clear(idAndAccessContextString => idAndAccessContextString._1 == userId) + userCache.clear { case (id, _) => id == userId }Optionally add a batched helper (outside the changed lines):
def removeUsersFromCache(userIds: Iterable[ObjectId]): Unit = { val ids = userIds.toSet userCache.clear { case (id, _) => ids.contains(id) } }If
AuthenticationController.logoutEverywherecurrently iterates one-by-one, switching to the batched helper would reduce cache traversal work under high user counts.frontend/javascripts/libs/toast.tsx (2)
3-3: Avoid importing all of lodash; import onlydebounceto cut bundle size.Switching to a focused import prevents pulling in the full lodash build.
Apply:
-import _ from "lodash"; +import debounce from "lodash/debounce";
248-259: Global debounce will suppress unrelated toasts; add a keyed variant to throttle per-message.The exported
showToastOncedebounces all toasts across the entire app for 60s, which can hide different, legitimate toasts fired within that window. KeepshowToastOncefor trivial cases, but add a keyed version that throttles by a semantic key (ideally the toast’s ownconfig.key).Minimal change within these lines (align with focused import above):
-export const showToastOnce = _.debounce( +export const showToastOnce = debounce( ( type: ToastStyle, message: React.ReactNode, config: ToastConfig = {}, details?: string | undefined, ) => { Toast[type](message, config, details); }, 60000, { leading: true }, );Then add a keyed helper (outside the changed lines):
// Throttle per logical key (defaults to config.key, falling back to type+stringified message) export const showToastOnceByKey = (() => { const debouncers = new Map<string, ReturnType<typeof debounce>>(); return ( type: ToastStyle, message: React.ReactNode, config: ToastConfig = {}, details?: string | undefined, ) => { const fallbackMessageKey = typeof message === "string" ? message : JSON.stringify({ t: type, m: "node" }); const key = config.key ?? `${type}:${fallbackMessageKey}`; let d = debouncers.get(key); if (!d) { d = debounce( (t: ToastStyle, m: React.ReactNode, c: ToastConfig, det?: string) => { Toast[t](m, { ...c, key }, det); // Clear debouncer so a new window starts after 60s debouncers.delete(key); }, 60000, { leading: true }, ); debouncers.set(key, d); } d(type, message, config, details); }; })();Call sites that already provide unique
config.keycan useshowToastOnceByKeyto prevent spam without globally silencing other toasts.app/models/user/User.scala (2)
168-170: Scope/ACL of findIdsByMultiUserId verified – safe to keep publicI ran the grep search for
findIdsByMultiUserIdand confirmed only two hits:
- Its declaration in
app/models/user/User.scala- Its single invocation in
AuthenticationController.logoutEverywhere(app/controllers/AuthenticationController.scala:963)
No other call sites exist, so there’s no unguarded bulk-ID exposure beyond your intended logout-everywhere flow.Next steps:
- If you leave it public (so the controller in a different package can call it), add a Scaladoc comment clarifying it’s only for cache eviction on logout-everywhere.
- Optional refactor: move this into a small service or companion in the
app.models.userpackage and mark itprivate[user](orprivate[models.user]); you’d then update the controller to use that service.
590-595: Suggestion: Restrict logout‐everywhere updates to active users & capture row countI checked the
cookieUnlessSignedOutEverywherelogic inCombinedAuthenticatorService.scala– it already rejects cookies whose last‐used time is on or before the user’sloggedOutEverywhereTime, defaulting toInstant.ZEROwhen unset. That confirms the cookie path is gated correctly by the new column.Next, I located where the token authenticator is wired up (using
tokenAuthenticatorService.retrieve). It does not perform the same timestamp check, so bearer‐token sessions would remain valid across a global logout. We should mirror the cookie check for tokens as well.• In
User.scala(lines 590–595), modify the UPDATE:- _ <- run( - q"""UPDATE webknossos.users SET loggedOutEverywhereTime = ${Instant.now} WHERE _multiUser = $multiUserId""".asUpdate) + updated <- run( + q"""UPDATE webknossos.users + SET loggedOutEverywhereTime = ${Instant.now} + WHERE _multiUser = $multiUserId + AND NOT isDeleted""".asUpdate) + logger.info(s"logoutEverywhere: updated $updated rows for multiUser=$multiUserId")• In the token‐validation path (e.g. wherever
tokenAuthenticatorService.retrieveis accepted), add the same “greater‐than” comparison againstloggedOutEverywhereTime, analogous tocookieUnlessSignedOutEverywhere.That ensures both cookies and tokens are invalidated when a user logs out everywhere.
conf/evolutions/139-logout-everywhere.sql (1)
1-24: Migration is sound and transactional; add small resilience improvements
- The migration wraps all changes in a transaction and asserts the prior schema version, preventing partial application.
- Views are dropped and recreated in the correct order around the new TIMESTAMPTZ column.
- An index on users(_multiUser) was already created in evolution 060-multiusers.sql, so no additional index DDL is needed here.
- To ease local/dev rollbacks, use IF EXISTS on the DROP VIEW statements:
-DROP VIEW webknossos.userInfos; -DROP VIEW webknossos.users_; +DROP VIEW IF EXISTS webknossos.userInfos; +DROP VIEW IF EXISTS webknossos.users_;unreleased_changes/8850.md (1)
1-6: Grammar nit: improve changelog phrasingConsider “log out of all devices” and clarify it’s accessible from Account Security.
-### Added -- Added the option to log out from all devices. +### Added +- Added an option to log out of all devices (Account Settings → Security).frontend/javascripts/admin/rest_api.ts (1)
2330-2330: Suppressing error toast for createShortLink — confirm UX intentSilencing toasts can avoid duplicate messaging, but ensure failures are still surfaced (e.g., inline form error or console).
If you want to keep global silence but log for troubleshooting:
- showErrorToast: false, + showErrorToast: false, + // on failure, Request.* helpers still reject the promise; callers should handle and log if neededfrontend/javascripts/admin/auth/change_email_view.tsx (1)
39-42: Make navigation resilient and update browser history after logoutEnsure the user is redirected to login even if the logoutEverywhere request fails (network flake) and prevent “Back” from re-entering the app with a stale page.
- await logoutUserEverywhere(); - Store.dispatch(logoutUserAction()); - navigate("/auth/login"); + try { + await logoutUserEverywhere(); + } finally { + Store.dispatch(logoutUserAction()); + navigate("/auth/login", { replace: true }); + }Optional: If logoutEverywhere fails, show a secondary toast (“Couldn’t complete ‘logout everywhere’; you’ve been logged out of this device.”) instead of the generic “changing the email address” error.
frontend/javascripts/libs/request.ts (1)
262-264: Type flow: catch handler now narrows the promise to Promise (from the helper).Because handleError is declared to return Promise, fetchPromise’s inferred type after .catch(...) becomes Promise. It still rejects correctly, but you lose value typing for callers. Consider adjusting the helper to return Promise to reflect that it always rejects (see suggested diff in handle_request_error_helper.tsx).
conf/evolutions/reversions/139-logout-everywhere.sql (2)
5-7: Make drops idempotent and robust.Using IF EXISTS helps when environments drift or when partial rollbacks happen. Consider adding CASCADE only if you expect dependent objects that can be safely recreated below.
-DROP VIEW webknossos.userInfos; -DROP VIEW webknossos.users_; +DROP VIEW IF EXISTS webknossos.userInfos; +DROP VIEW IF EXISTS webknossos.users_;
10-20: Prefer explicit column lists in views.SELECT * in views makes them fragile to table shape changes. Listing columns for webknossos.users_ and userInfos improves forward/backward compatibility during rolling deploys.
frontend/javascripts/libs/handle_request_error_helper.tsx (3)
5-10: This helper always rejects; type it as Promise (not Promise).The function never resolves; it only rejects. Returning Promise preserves downstream typing (see Request.triggerRequest catch). Minor but useful for type safety.
-export const handleError = async ( +export const handleError = async ( requestedUrl: string, showErrorToast: boolean, doInvestigate: boolean, error: Response | Error, -): Promise<void> => { +): Promise<never> => { @@ - return Promise.reject(error); + return Promise.reject(error);Also applies to: 84-85
17-29: 401 UX: consider redirecting to login (optional).For global logout and expired sessions, showing a sticky “Refresh” toast is okay. Given the “logout everywhere” feature, you may want to also route to /auth/login (or dispatch logout) on 401 to reduce confusion. If you keep the toast, that’s fine; just a UX suggestion.
78-82: Minor: ensure error is an Error before mutating message.The instanceof check ensures it’s not a Response, but extremely rarely non-Error throwables can occur. Narrow with instanceof Error for safety; otherwise ignore.
- if (!(error instanceof Response)) { + if (!(error instanceof Response) && error instanceof Error) { error.message += ` - Url: ${requestedUrl}`; }frontend/javascripts/admin/account/account_security_view.tsx (3)
185-195: Simplify handleLogout with async/await; close modal; avoid duplicate toasts.Use await/try-catch, close the modal on success, and let Request’s centralized handler show errors instead of adding another toast.
- async function handleLogout() { - logoutUserEverywhere() - .then(() => { - Store.dispatch(logoutUserAction()); - navigate("/login"); - }) - .catch((error) => { - Toast.error("Failed to log out. See console for more details"); - console.error("Logout failed:", error); - }); - } + async function handleLogout() { + try { + await logoutUserEverywhere(); + setShowConfirmLogoutModal(false); + Store.dispatch(logoutUserAction()); + navigate("/auth/login"); + } catch (error) { + console.error("Logout failed:", error); + // Centralized Request handler already surfaced the error + } + }
172-178: UX nit: signal destructive action.Mark the “Log out on all devices” button as danger to convey impact.
- <Button type="default" onClick={() => setShowConfirmLogoutModal(true)}> + <Button type="default" danger onClick={() => setShowConfirmLogoutModal(true)}> Log out on all devices </Button>
206-209: Keying by title can collide with i18n.Using item.title as React key risks collisions when labels change. Consider adding a stable id to SettingsCardProps and key by that. Optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
test/db/users.csvis excluded by!**/*.csv
📒 Files selected for processing (19)
app/controllers/AuthenticationController.scala(1 hunks)app/models/user/User.scala(5 hunks)app/models/user/UserService.scala(1 hunks)app/security/CombinedAuthenticatorService.scala(2 hunks)conf/evolutions/139-logout-everywhere.sql(1 hunks)conf/evolutions/reversions/139-logout-everywhere.sql(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/account/account_security_view.tsx(3 hunks)frontend/javascripts/admin/account/account_settings_view.tsx(2 hunks)frontend/javascripts/admin/auth/change_email_view.tsx(3 hunks)frontend/javascripts/admin/auth/verify_email_view.tsx(1 hunks)frontend/javascripts/admin/rest_api.ts(2 hunks)frontend/javascripts/libs/handle_http_status.ts(1 hunks)frontend/javascripts/libs/handle_request_error_helper.tsx(1 hunks)frontend/javascripts/libs/request.ts(2 hunks)frontend/javascripts/libs/toast.tsx(2 hunks)frontend/javascripts/router/router.tsx(2 hunks)tools/postgres/schema.sql(2 hunks)unreleased_changes/8850.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T07:33:45.681Z
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
Learning: In the webknossos project, the Request module has comprehensive built-in error handling. The triggerRequest method defaults to showErrorToast: true, automatically catches errors with .catch(), and displays user notifications via Toast.error() or Toast.messages() in the handleError method. Functions using Request methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Applied to files:
frontend/javascripts/libs/handle_request_error_helper.tsxfrontend/javascripts/libs/request.ts
🧬 Code graph analysis (7)
app/controllers/AuthenticationController.scala (2)
app/models/user/User.scala (2)
logOutEverywhereByMultiUserId(590-595)findIdsByMultiUserId(168-171)app/models/user/UserService.scala (1)
removeUserFromCache(220-223)
app/security/CombinedAuthenticatorService.scala (2)
util/src/main/scala/com/scalableminds/util/time/Instant.scala (3)
Instant(14-45)Instant(47-103)zero(52-52)app/models/user/UserService.scala (1)
retrieve(272-275)
frontend/javascripts/admin/auth/change_email_view.tsx (1)
frontend/javascripts/admin/rest_api.ts (1)
logoutUserEverywhere(157-159)
frontend/javascripts/libs/handle_request_error_helper.tsx (3)
frontend/javascripts/admin/datastore_health_check.ts (1)
pingMentionedDataStores(105-107)frontend/javascripts/libs/toast.tsx (1)
showToastOnce(248-259)frontend/javascripts/libs/handle_http_status.ts (1)
ServerErrorMessage(1-3)
app/models/user/User.scala (4)
util/src/main/scala/com/scalableminds/util/time/Instant.scala (4)
Instant(14-45)Instant(47-103)fromSql(58-58)now(48-48)app/utils/sql/SimpleSQLDAO.scala (1)
run(28-48)app/utils/sql/SqlInterpolation.scala (3)
q(19-38)as(53-73)asUpdate(73-73)app/utils/sql/SecuredSQLDAO.scala (1)
existingCollectionName(16-16)
frontend/javascripts/libs/request.ts (1)
frontend/javascripts/libs/handle_request_error_helper.tsx (1)
handleError(5-85)
frontend/javascripts/admin/account/account_security_view.tsx (3)
frontend/javascripts/admin/rest_api.ts (1)
logoutUserEverywhere(157-159)frontend/javascripts/admin/account/helpers/settings_card.tsx (2)
SettingsCardProps(4-10)SettingsCard(12-38)frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)
🪛 LanguageTool
unreleased_changes/8850.md
[grammar] ~1-~1: There might be a mistake here.
Context: ### Added - Added the option to log out from all dev...
(QB_NEW_EN)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (16)
frontend/javascripts/libs/handle_http_status.ts (1)
1-3: No stale imports ofServerErrorMessagedetectedI searched the codebase for any remaining imports of
ServerErrorMessagefromlibs/requestand found none—every consumer now pulls it fromhandle_http_status.ts. This centralization is safe to merge.• Optional: if you’d like to preserve backward-compatibility, you can re-export the type in
libs/request/index.ts:// libs/request/index.ts export type { ServerErrorMessage } from "libs/handle_http_status";frontend/javascripts/admin/auth/verify_email_view.tsx (1)
3-3: Import path update looks correct.Type-only import is appropriate and keeps bundling lean. No further action from this change.
frontend/javascripts/admin/account/account_settings_view.tsx (1)
10-11: Breadcrumb label rename to “Security” is consistent with the new page.Looks good and aligns with the route/name changes elsewhere.
app/models/user/User.scala (2)
122-123: Row → model mapping for loggedOutEverywhereTime is accurateMapping from r.loggedouteverywheretime to Instant.fromSql is consistent with other timestamp fields.
43-45: No direct JSON serialization of User –loggedOutEverywhereTimeis not exposedOur searches did not find any implicit Writes/Format or macro-based JSON serializers for the
Usercase class, nor any directJson.toJson(user)calls in controllers or services. All API endpoints invoke custom serializers (e.g.userService.publicWrites,userService.compactWrites, or write against DTOs such asUserCompactInfoorUserAccessAnswer), none of which include the newloggedOutEverywhereTimefield.• No
implicit val/defforWrites[User],Format[User], etc., inapp/models/user/User.scalaor elsewhere.
• No directJson.toJson(user)invocations in controllers—only DTO-based serialization.
• Existing DTOs (UserCompactInfo,UserAccessAnswer, etc.) and their companion serializers won’t include the new timestamp.Since there’s no accidental macro serialization of the full
Usermodel,loggedOutEverywhereTimewill not be sent in any public JSON response. If at any point you add a genericWrites[User], remember to explicitly omit this field in your public‐facing DTOs or write definitions.tools/postgres/schema.sql (1)
24-24: Schema bump and users.loggedOutEverywhereTime mirror the migration — LGTMThe schemaVersion = 139 and the new TIMESTAMPTZ column align with the evolution. The preexisting index on users(_multiUser) covers the UPDATE predicate.
Also applies to: 413-414
frontend/javascripts/admin/rest_api.ts (2)
154-155: Switching logout to POST matches backend route — goodExplicit POST avoids relying on defaults and aligns with typical CSRF defenses.
157-159: New API logoutUserEverywhere — good minimal surfaceStraightforward wrapper to the new endpoint.
frontend/javascripts/router/router.tsx (1)
468-470: Route swap looks goodImporting AccountSecurityView and wiring /account/security is consistent with the UI rename. No issues.
conf/webknossos.latest.routes (1)
35-36: No GET /auth/logout usage detected; routes update is safe
- Ripgrep search confirmed no client-side anchors or GET requests targeting
/auth/logoutoutside of the routes file.- Frontend’s
logoutUser()(infrontend/javascripts/admin/rest_api.ts) issues a POST to/api/auth/logoutas intended.- There are no
<Link>s, router entries, or other code paths invoking GET/auth/logout.app/security/CombinedAuthenticatorService.scala (1)
83-93: No JodaTime usage in Silhouette 10.0.3 – Java Date API in useI’ve confirmed that the project is using Silhouette 10.0.3 (defined in
project/Dependencies.scala) and, per the Silhouette changelog, JodaTime was removed in version 9.0.0 and replaced with the Java Date API (github.com). As a result,cookie.lastUsedDateTimeis ajava.timetype, so callingtoInstant.toEpochMilliis correct and no change is needed here.frontend/javascripts/libs/request.ts (2)
9-9: Good move: centralize error handling via helper.Importing handleError from a dedicated helper keeps Request lean and avoids circular deps. No further concerns here.
255-256: No-op functional change; fine to keep.Wrapping handleStatus in an arrow is semantically identical to then(handleStatus). If this was to aid debugging or typings, LGTM.
frontend/javascripts/libs/handle_request_error_helper.tsx (2)
1-86: Centralization of HTTP error handling looks good.Dynamic import to avoid cycles, datastore health ping, and consolidated toasts align with previous Request behavior. Nice separation of concerns.
1-86: No staleServerErrorMessageimports found
Ran a ripgrep search for anyServerErrorMessageimports fromlibs/requestand found zero occurrences. Everything is correctly pointing tolibs/handle_http_status.frontend/javascripts/admin/account/account_security_view.tsx (1)
1-236: Overall: solid introduction of Security view and logout-everywhere flow.The modal, cards, and action wiring look clean. Address the route inconsistency and small flow issues above and this should be ready.
| return error.text().then( | ||
| (text) => { | ||
| try { | ||
| const json = JSON.parse(text); | ||
|
|
||
| // Propagate HTTP status code for further processing down the road | ||
| if (error.status != null) { | ||
| json.status = error.status; | ||
| } | ||
|
|
||
| const messages = json.messages.map((message: ServerErrorMessage[]) => ({ | ||
| ...message, | ||
| key: json.status.toString(), | ||
| })); | ||
| if (showErrorToast) { | ||
| Toast.messages(messages); // Note: Toast.error internally logs to console | ||
| } else { | ||
| console.error(messages); | ||
| } | ||
| // Check whether the error chain mentions an url which belongs | ||
| // to a datastore. Then, ping the datastore | ||
| pingMentionedDataStores(text); | ||
|
|
||
| /* eslint-disable-next-line prefer-promise-reject-errors */ | ||
| return Promise.reject({ ...json, url: requestedUrl }); | ||
| } catch (_jsonError) { | ||
| if (showErrorToast) { | ||
| Toast.error(text); // Note: Toast.error internally logs to console | ||
| } else { | ||
| console.error(`Request failed for ${requestedUrl}:`, text); | ||
| } | ||
|
|
||
| /* eslint-disable-next-line prefer-promise-reject-errors */ | ||
| return Promise.reject({ | ||
| errors: [text], | ||
| status: error.status != null ? error.status : -1, | ||
| url: requestedUrl, | ||
| }); | ||
| } | ||
| }, | ||
| (textError) => { | ||
| Toast.error(textError.toString()); | ||
| return Promise.reject(textError); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: incorrect typing/mapping of json.messages; add null guards.
json.messages.map((message: ServerErrorMessage[]) => ...) treats each message as an array. Spreading an array into an object produces numeric keys, which is incorrect for Toast.messages. Also, json.messages may be absent or not an array. Fix the annotation and add a safe fallback.
- try {
- const json = JSON.parse(text);
+ try {
+ const json = JSON.parse(text);
@@
- const messages = json.messages.map((message: ServerErrorMessage[]) => ({
- ...message,
- key: json.status.toString(),
- }));
+ const rawMessages = Array.isArray(json.messages) ? json.messages : [];
+ const messages = rawMessages.map((message: ServerErrorMessage) => ({
+ ...message,
+ key: String(json.status),
+ }));
+ // If server did not provide messages, fall back to a single generic error
+ const messagesOrFallback =
+ messages.length > 0 ? messages : [{ error: json.error ?? text, key: String(json.status) }];
- if (showErrorToast) {
- Toast.messages(messages); // Note: Toast.error internally logs to console
+ if (showErrorToast) {
+ Toast.messages(messagesOrFallback); // Note: Toast.error internally logs to console
} else {
- console.error(messages);
+ console.error(messagesOrFallback);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return error.text().then( | |
| (text) => { | |
| try { | |
| const json = JSON.parse(text); | |
| // Propagate HTTP status code for further processing down the road | |
| if (error.status != null) { | |
| json.status = error.status; | |
| } | |
| const messages = json.messages.map((message: ServerErrorMessage[]) => ({ | |
| ...message, | |
| key: json.status.toString(), | |
| })); | |
| if (showErrorToast) { | |
| Toast.messages(messages); // Note: Toast.error internally logs to console | |
| } else { | |
| console.error(messages); | |
| } | |
| // Check whether the error chain mentions an url which belongs | |
| // to a datastore. Then, ping the datastore | |
| pingMentionedDataStores(text); | |
| /* eslint-disable-next-line prefer-promise-reject-errors */ | |
| return Promise.reject({ ...json, url: requestedUrl }); | |
| } catch (_jsonError) { | |
| if (showErrorToast) { | |
| Toast.error(text); // Note: Toast.error internally logs to console | |
| } else { | |
| console.error(`Request failed for ${requestedUrl}:`, text); | |
| } | |
| /* eslint-disable-next-line prefer-promise-reject-errors */ | |
| return Promise.reject({ | |
| errors: [text], | |
| status: error.status != null ? error.status : -1, | |
| url: requestedUrl, | |
| }); | |
| } | |
| }, | |
| (textError) => { | |
| Toast.error(textError.toString()); | |
| return Promise.reject(textError); | |
| }, | |
| ); | |
| } | |
| return error.text().then( | |
| (text) => { | |
| try { | |
| const json = JSON.parse(text); | |
| // Propagate HTTP status code for further processing down the road | |
| if (error.status != null) { | |
| json.status = error.status; | |
| } | |
| const rawMessages = Array.isArray(json.messages) ? json.messages : []; | |
| const messages = rawMessages.map((message: ServerErrorMessage) => ({ | |
| ...message, | |
| key: String(json.status), | |
| })); | |
| // If server did not provide messages, fall back to a single generic error | |
| const messagesOrFallback = | |
| messages.length > 0 | |
| ? messages | |
| : [{ error: json.error ?? text, key: String(json.status) }]; | |
| if (showErrorToast) { | |
| Toast.messages(messagesOrFallback); // Note: Toast.error internally logs to console | |
| } else { | |
| console.error(messagesOrFallback); | |
| } | |
| // Check whether the error chain mentions an url which belongs | |
| // to a datastore. Then, ping the datastore | |
| pingMentionedDataStores(text); | |
| /* eslint-disable-next-line prefer-promise-reject-errors */ | |
| return Promise.reject({ ...json, url: requestedUrl }); | |
| } catch (_jsonError) { | |
| if (showErrorToast) { | |
| Toast.error(text); // Note: Toast.error internally logs to console | |
| } else { | |
| console.error(`Request failed for ${requestedUrl}:`, text); | |
| } | |
| /* eslint-disable-next-line prefer-promise-reject-errors */ | |
| return Promise.reject({ | |
| errors: [text], | |
| status: error.status != null ? error.status : -1, | |
| url: requestedUrl, | |
| }); | |
| } | |
| }, | |
| (textError) => { | |
| Toast.error(textError.toString()); | |
| return Promise.reject(textError); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In frontend/javascripts/libs/handle_request_error_helper.tsx around lines 30 to
75, json.messages is incorrectly typed/mapped as ServerErrorMessage[] and may be
missing or not an array; change the mapping to treat each item as a single
ServerErrorMessage (not an array), add a guard using
Array.isArray(json.messages) (falling back to an empty array) before mapping,
and ensure each mapped message is an object (spread the message only when it is
an object) and assign a stable key (e.g. json.status?.toString() or a fallback)
so Toast.messages always receives a proper array of objects; update downstream
rejects to use the safe messages fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/router/router.tsx (1)
378-379: Use replace on Navigate to avoid polluting browser history for compat redirects.These are pure compatibility redirects; adding replace prevents a back-button loop to the legacy URL.
Apply:
-<Route path="/auth/changePassword" element={<Navigate to="/account/security" />} /> -<Route path="/account/password" element={<Navigate to="/account/security" />} /> +<Route path="/auth/changePassword" element={<Navigate to="/account/security" replace />} /> +<Route path="/account/password" element={<Navigate to="/account/security" replace />} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
frontend/javascripts/admin/account/account_security_view.tsx(3 hunks)frontend/javascripts/admin/auth/verify_email_view.tsx(1 hunks)frontend/javascripts/libs/handle_http_status.ts(0 hunks)frontend/javascripts/libs/handle_request_error_helper.tsx(1 hunks)frontend/javascripts/router/router.tsx(3 hunks)frontend/javascripts/types/api_types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/libs/handle_http_status.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/admin/auth/verify_email_view.tsx
- frontend/javascripts/libs/handle_request_error_helper.tsx
- frontend/javascripts/admin/account/account_security_view.tsx
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/router/router.tsx (2)
50-50: Verification Complete: No staleAccountPasswordViewimports; legacy routes are intentionally redirectedI’ve confirmed that:
- There are no remaining imports or usages of
AccountPasswordViewacross.ts/.tsxfiles.- No files matching
account_password_view.*exist in the repo.- The old front-end routes (
/auth/changePassword,/account/password) still appear inrouter.tsxbut correctly redirect to/account/securityfor backward compatibility.- The call to
/api/auth/changePasswordinadmin/rest_api.tsis the live backend endpoint (not a view route) and does not require removal.No further cleanup is needed here.
467-471: Verified legacy redirects; optional default‐to‐security remains safe
- In
frontend/javascripts/router/router.tsxlines 378–379, we already have backwards-compatibility routes:<Route path="/auth/changePassword" element={<Navigate to="/account/security" />} /> <Route path="/account/password" element={<Navigate to="/account/security" />} />- No other hard-coded links to
/account/passwordor/auth/changePasswordwere found in TS/TSX files or elsewhere in the repo.- If you’d like “Security” to be the default tab under
/account, you can apply the optional refactor:-<Route index element={<Navigate to="profile" />} /> +<Route index element={<Navigate to="security" replace />} />- Please manually confirm that any menu items or command-palette entries pointing to the old password view now use
/account/security.frontend/javascripts/types/api_types.ts (2)
1303-1306: Centralizing ServerErrorMessage here looks goodMoving the error payload type into api_types.ts improves discoverability and reuse across the frontend.
1303-1306: Add JSDoc to ServerErrorMessage and confirm no lingering imports• TSConfig’s
pathssetting maps"*"→"./frontend/javascripts/*", so the canonical import for this type isimport type { ServerErrorMessage } from 'types/api_types';No explicit
types/*alias is defined, but the wildcard covers it.• Verified with
ripgrepthat there are no remaining imports ofServerErrorMessagefrom:
libs/request- the literal path
frontend/javascripts/types/api_types.ts- any other source
(i.e., no call sites require updating)Apply this diff to add documentation:
-export type ServerErrorMessage = { - error: string; -}; +/** + * JSON error body returned by backend endpoints. + * Example: HTTP 4xx/5xx with payload { "error": "<human-readable message>" }. + * Only use where the server guarantees this shape. + */ +export type ServerErrorMessage = { + error: string; +};
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/router/router.tsx (3)
50-50: Switch to AccountSecurityView import — consider lazy-loading for parity and bundle size.The direct import works, but this view is only needed on the account settings route. Consider lazy-loading it (you already use loadable elsewhere) to keep the router bundle lean.
Apply this diff within the selected range to drop the static import:
-import AccountSecurityView from "admin/account/account_security_view"; +// (lazy-load AccountSecurityView below to reduce initial bundle size)Add this near your other loadables (e.g., after the existing AsyncWorkflow declarations):
const AccountSecurityView = loadable<EmptyObject>(() => import("admin/account/account_security_view"), );
376-380: Back-compat redirects look good; minor consistency nit: add replace to /login.Using replace prevents polluting history. You already use it for other back-compat redirects; mirror that on /login for consistency.
- <Route path="/login" element={<Navigate to="/auth/login" />} /> + <Route path="/login" element={<Navigate to="/auth/login" replace />} />Note: If any of the legacy URLs carried query params (e.g., ?redirect=...), those are dropped by static declarations. If preserving search params is important, we can convert these to small wrapper components that read location.search and forward it.
386-390: Legacy signup/register references audit — ripgrep results & next stepsThe provided ripgrep search returned only intentional references in the JS/TS codebase:
- API calls in
frontend/javascripts/admin
•/api/auth/registerin registration_form_generic.tsx
•/api/auth/changePasswordin rest_api.ts
•/api/auth/webauthn/register/...in webauthn.ts- UI components now correctly linking to
/auth/signup
•<Button href="https://github.com/auth/signup"…>in welcome_toast.tsx
•<Link to="/auth/signup">in login_form.tsx- Asset references in stylesheets
•signup.svgin_drawings.less- Server route definitions
• POST/auth/register,/auth/changePassword, etc., inconf/webknossos.latest.routes- Changelog entries noting the explicit
/signuprouteNo unexpected or stale deep‐linkable routes (
/signup,/register,/auth/register) were found in the JS/TS code.Please extend this verification to cover:
- Documentation files (e.g.
*.md)- Email templates or view files (e.g. under
app/views/email*or similar)If any remaining links to the old paths are intentional, consider annotating them with a clarifying comment or updating them to
/auth/signupto avoid user‐facing 404s when opening deep links.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/javascripts/router/router.tsx(3 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (1)
frontend/javascripts/router/router.tsx (1)
469-469: New “/account/security” nested route under SecuredRoute — LGTM.Matches the rename and aligns with the new AccountSecurityView. The back-compat redirect from /account/password covers legacy deep links.
@knollengewaechs This CodeRabit comment might be a good idea. Otherwise, one when trying to navigate back, one is stuck in a endless "back" <-> "redirect" loop. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/AuthenticationController.scala (1)
961-968: Discard the current authenticator and avoid map-for-side-effects in logoutEverywhereReturn an authenticator-discard result so the browser removes the cookie immediately, and use foreach for side effects. This aligns with existing patterns in changePassword and prevents stale cookies on the initiating tab. Also logs are useful for auditability.
def logoutEverywhere: Action[AnyContent] = sil.SecuredAction.async { implicit request => for { _ <- userDAO.logOutEverywhereByMultiUserId(request.identity._multiUser) userIds <- userDAO.findIdsByMultiUserId(request.identity._multiUser) - _ = userIds.map(userService.removeUserFromCache) + _ = userIds.foreach(userService.removeUserFromCache) _ <- tokenDAO.deleteDataStoreTokensForMultiUser(request.identity._multiUser) - } yield Ok + _ = logger.info(s"Multiuser ${request.identity._multiUser} issued logout-everywhere.") + authenticatorResult <- Fox.fromFuture(combinedAuthenticatorService.discard(request.authenticator, Ok)) + } yield authenticatorResult }Follow-up thought: If your threat model requires it, consider whether non-DataStore bearer tokens (e.g., ResetPassword) should also be invalidated for this multi-user. Happy to sketch a DAO variant if desired.
🧹 Nitpick comments (1)
app/security/Token.scala (1)
137-148: Bulk soft-delete for DataStore tokens — add NOT isDeleted; providerKey confirmed to be user.id; users reference optionalVerified findings (short):
- DataStore tokens are created from user.loginInfo (createAndInitDataStoreTokenForUser -> createAndInit(user.loginInfo, TokenType.DataStore)), and code expects providerKey to be an ObjectId — providerKey = user._id.
- tokens schema exposes a view webknossos.tokens_ (SELECT * FROM webknossos.tokens WHERE NOT isDeleted), so skipping already-deleted rows is safe and avoids redundant writes.
- The hard-coded webknossos.users_ subquery is brittle; consider using the User DAO's existingCollectionName or the stable view webknossos.users if you want to future-proof it.
Locations to update / review:
- app/security/Token.scala — deleteDataStoreTokensForMultiUser (around lines 137–148).
- Evidence: app/security/WebknossosBearerTokenAuthenticatorService.scala (createAndInitDataStoreTokenForUser/createAndInit), app/models/user/UserService.scala (createLoginInfo uses ObjectId), tools/postgres/schema.sql (tokens_ view).
Apply this minimal diff to avoid redundant updates:
- WHERE tokenType = ${TokenType.DataStore} + WHERE tokenType = ${TokenType.DataStore} + AND NOT isDeletedOptional: replace the hard-coded "FROM webknossos.users_" subquery with the user DAO's existingCollectionName or "webknossos.users" view to reduce brittleness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/controllers/AuthenticationController.scala(2 hunks)app/security/Token.scala(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/security/Token.scala (2)
app/utils/sql/SqlInterpolation.scala (2)
q(19-38)asUpdate(73-73)app/security/TokenType.scala (1)
TokenType(5-8)
app/controllers/AuthenticationController.scala (3)
app/models/user/User.scala (2)
logOutEverywhereByMultiUserId(590-595)findIdsByMultiUserId(168-171)app/models/user/UserService.scala (1)
removeUserFromCache(220-223)app/security/Token.scala (1)
deleteDataStoreTokensForMultiUser(137-149)
⏰ 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). (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (1)
app/controllers/AuthenticationController.scala (1)
204-204: DI addition of TokenDAO looks goodWiring TokenDAO here is appropriate for the logout-everywhere flow and matches its usage below.
|
@hotzenklotz there were quite some changes after your review. I added |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/javascripts/router/router.tsx (2)
376-378: External docs redirect: consider a tiny ExternalRedirect component.
Using Navigate to an absolute URL is okay, but a dedicated component makes intent explicit and avoids any router nuances.Example:
// add once in this file (or a shared utils file) import { useEffect } from "react"; function ExternalRedirect({ href }: { href: string }) { useEffect(() => { window.location.replace(href); }, [href]); return null; } // usage <Route path="/help/keyboardshortcuts" element={ <ExternalRedirect href="https://docs.webknossos.org/webknossos/ui/keyboard_shortcuts.html" /> } />
381-395: Nit: also use replace on the /login compat redirect for consistency.
All other compat redirects here use replace — mirror it for /login to avoid history pollution.- <Route path="/login" element={<Navigate to="/auth/login" />} /> + <Route path="/login" element={<Navigate to="/auth/login" replace />} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/javascripts/router/router.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/router/router.tsx (5)
frontend/javascripts/admin/organization/organization_overview_view.tsx (1)
OrganizationOverviewView(25-210)frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
OrganizationNotificationsView(21-122)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
OrganizationDangerZoneView(10-70)frontend/javascripts/admin/auth/accept_invite_view.tsx (1)
AcceptInviteView(15-117)frontend/javascripts/admin/auth/verify_email_view.tsx (1)
VerifyEmailView(47-92)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (7)
frontend/javascripts/router/router.tsx (7)
50-50: Switched to AccountSecurityView import — good alignment with new /account/security route.
172-172: Compat redirect now uses replace — prevents back-button loops.
278-278: Index redirect for dataset settings uses replace — nice touch.
357-360: Compat redirect /organizations/:organizationId → /organization with replace — looks good.
369-369: Organization index redirect uses replace — LGTM.
472-472: Account index redirect uses replace — good.
474-474: New /account/security route wired to AccountSecurityView — matches feature intent.
|
Frontend changes LGTM |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/router/router.tsx (1)
386-386: Nit: also add replace to /login → /auth/login for consistency.Keeps browser history clean like other compat redirects.
- <Route path="/login" element={<Navigate to="/auth/login" />} /> + <Route path="/login" element={<Navigate to="/auth/login" replace />} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
conf/application.conf(1 hunks)frontend/javascripts/router/router.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/router/router.tsx (5)
frontend/javascripts/admin/organization/organization_overview_view.tsx (1)
OrganizationOverviewView(25-210)frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
OrganizationNotificationsView(21-122)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
OrganizationDangerZoneView(10-70)frontend/javascripts/admin/auth/accept_invite_view.tsx (1)
AcceptInviteView(15-117)frontend/javascripts/admin/auth/verify_email_view.tsx (1)
VerifyEmailView(47-92)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (10)
conf/application.conf (1)
149-149: Confirm intent: isWkorgInstance set to true affects routing and onboarding semantics.If this was only for the temporary dev instance, consider reverting in the default config and overriding via environment- or profile-specific config to avoid surprising local/prod defaults.
frontend/javascripts/router/router.tsx (9)
35-36: Good: import redirect for data-router redirects.Enables loader-based redirects like the publication route below.
51-51: Good: swap to AccountSecurityView.Matches the new canonical /account/security route.
173-173: LGTM: replace on compatibility redirect.Prevents back-button loops for /reports/openTasks.
279-279: LGTM: index redirect uses replace.Avoids extra history entries in dataset settings.
358-361: LGTM: org id → /organization with replace.Cleaner history for legacy org URLs.
370-370: LGTM: organization index → overview with replace.
382-396: LGTM: auth/password/register compat redirects now use replace.Consistent with the “don’t pollute history” goal.
436-439: Good fix: loader-based param redirect for /publication/:id.Avoids literal “:id” path; aligns with RR data-router best practices.
476-478: LGTM: /account index → profile (replace) and security route wiring.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/ShortLinkController.scala (1)
26-27: Host check via startsWith is unsafe (open-redirect/phishing risk)String prefix checks allow crafted domains (e.g., https://example.org.attacker.com) and userinfo tricks. Validate scheme/host via URI parsing or, at minimum, enforce a trailing “/” boundary.
Robust fix:
+import java.net.URI +import scala.util.Try @@ - _ <- Fox.fromBool(longLink.startsWith(wkConf.Http.uri)) ?~> "Could not generate short link: URI does not match" + // Only allow short links to our own host (scheme + host match). Treat invalid URIs as mismatch. + val baseUri = new URI(wkConf.Http.uri) + val targetOk = Try(new URI(longLink)).toOption.exists { u => + u.isAbsolute && + Option(u.getScheme).contains(baseUri.getScheme) && + Option(u.getHost).contains(baseUri.getHost) + } + _ <- Fox.fromBool(targetOk) ?~> "Could not generate short link: URI host/scheme do not match"Minimal mitigation (if avoiding URI parse now):
- _ <- Fox.fromBool(longLink.startsWith(wkConf.Http.uri)) ?~> "Could not generate short link: URI does not match" + _ <- Fox.fromBool(longLink.startsWith(wkConf.Http.uri.stripSuffix(\"/\") + \"/\")) ?~> "Could not generate short link: URI does not match"Consider adding rate limiting if anonymous access is ever intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/controllers/ShortLinkController.scala(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/ShortLinkController.scala (2)
app/controllers/AnnotationPrivateLinkController.scala (1)
create(86-99)util/src/main/scala/com/scalableminds/util/mvc/ExtendedController.scala (1)
validateJson(204-209)
⏰ 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: frontend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/router/router.tsx (3)
173-173: Compat redirect uses replace — consider loader-based redirect for consistency.
Works as-is; using a loader avoids a render hop and aligns with other redirects in this file.Apply if you want parity with other routes:
- <Route path="/reports/openTasks" element={<Navigate to="/reports/availableTasks" replace />} /> + <Route path="/reports/openTasks" loader={() => redirect("/reports/availableTasks")} />
358-361: Organizations legacy redirect drops the organizationId.
If users deep-link to a specific org, silently ignoring the id might be confusing. Optional: switch to the requested org before redirecting.Example component-based approach:
// near other helpers import { useEffect } from "react"; import { useParams, Navigate } from "react-router-dom"; import { switchToOrganization } from "admin/organization/organization_utils"; function OrganizationLegacyRedirect() { const { organizationId = "" } = useParams(); useEffect(() => { if (organizationId) switchToOrganization(organizationId); }, [organizationId]); return <Navigate to="/organization" replace />; } // route: // <Route path="/organizations/:organizationId" element={<OrganizationLegacyRedirect />} />
380-385: Addreplaceto the/loginredirect
Ensure the<Navigate>for/loginusesreplaceto match other compat redirects and prevent back-button loops.- <Route path="/login" element={<Navigate to="/auth/login" />} /> + <Route path="/login" element={<Navigate to="/auth/login" replace />} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/javascripts/router/router.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/router/router.tsx (5)
frontend/javascripts/admin/organization/organization_overview_view.tsx (1)
OrganizationOverviewView(25-210)frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
OrganizationNotificationsView(21-122)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
OrganizationDangerZoneView(10-70)frontend/javascripts/admin/auth/accept_invite_view.tsx (1)
AcceptInviteView(15-117)frontend/javascripts/admin/auth/verify_email_view.tsx (1)
VerifyEmailView(47-92)
⏰ 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: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (8)
frontend/javascripts/router/router.tsx (8)
35-36: Importing redirect is correct for data-router redirects.
The usage below looks consistent.
51-51: Switch to AccountSecurityView import looks good.
Matches the new “Security” route below.
279-285: Index redirect to “data” with replace is correct.
No issues spotted.
370-374: Index redirect to organization/overview with replace is correct.
All good.
376-378: External docs redirect via loader — nice fix.
Addresses prior concern with absolute-URL .
390-395: Signup/register compat redirects now use replace — good.
Prevents polluting history.
434-437: Param-preserving publication redirect via loader — correct.
Fixes the earlier “:id not interpolated” bug.
474-478: Account index redirect and Security route wiring look correct.
Matches the new “Logout everywhere” UI location.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/router/router.tsx (1)
376-378: Fixed: external redirect now via loader.This addresses prior feedback about to absolute URLs. ✅
🧹 Nitpick comments (3)
frontend/javascripts/router/router.tsx (3)
279-279: Nested index redirect OK; optional: use loader-based redirect.Current Navigate works; consider a loader for parity with other redirects.
- <Route index element={<Navigate to="data" replace />} /> + <Route index loader={() => redirect("data")} />
358-361: Minor: avoid double-hop by redirecting straight to overview.Right now it goes /organizations/:id → /organization → /organization/overview. Jump directly.
- <Route - path="/organizations/:organizationId" - element={<Navigate to="/organization" replace />} - /> + <Route + path="/organizations/:organizationId" + element={<Navigate to="/organization/overview" replace />} + />
435-437: Param redirect looks correct; encode the id (nit).Guard against odd chars in ids.
- loader={({ params }) => redirect(`/publications/${params.id}`)} + loader={({ params }) => redirect(`/publications/${encodeURIComponent(params.id ?? "")}`)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/javascripts/router/router.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/router/router.tsx (3)
frontend/javascripts/admin/organization/organization_overview_view.tsx (1)
OrganizationOverviewView(25-210)frontend/javascripts/admin/organization/organization_notifications_view.tsx (1)
OrganizationNotificationsView(21-122)frontend/javascripts/admin/organization/organization_danger_zone_view.tsx (1)
OrganizationDangerZoneView(10-70)
⏰ 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: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (8)
frontend/javascripts/router/router.tsx (8)
35-35: Good: switched to data-routerredirect.Import looks correct for loader-based redirects.
173-173: Compat redirect usesreplace— prevents back-button loops.Looks good.
370-370: Good: canonicalizes /organization to overview withreplace.Prevents history bloat.
380-380: Good: auth-token compat redirect withreplace.No concerns.
382-384: Good: legacy auth routes consolidated to Account Security/Login withreplace.Matches the new security page.
390-394: Good: signup/register legacy routes usereplace.Keeps history clean; consistent with other compat redirects.
474-474: Good: /account index → profile withreplace.Consistent with other sections.
476-476: Security route wiring LGTM.New AccountSecurityView is mounted at the canonical path.
|
from my side this is ready to go! 🚀 |
### Steps to test: - Go to tree tab and enter long name or make tab really narrow. The circle next to the tree should stay round. ### TODOs: - [x] fix tree dot - [x] fix browsers back button for DS settings page (other PR: #8850) ------ - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md)
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
/auth/loginafterwards, just like in normal logoutIssues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)