feat(ui): add allowGuestDemoWorkspace flag to force login #12779
Conversation
… users to sign-in page for better user experience
…ation to improve user experience refactor(use-sign-out.ts): rename jumpToIndex to jumpToSignIn for clarity in navigation fix(index.tsx): clear last_workspace_id from localStorage on sign-out to prevent stale data fix(invite/index.tsx): ensure navigateHelper is used for sign-out navigation to maintain consistency fix(menu.tsx): utilize navigateHelper for sign-out navigation to enhance user flow
… demo workspace configuration This change introduces a check for the `allowGuestDemoWorkspace` configuration. If enabled, the user is redirected to the index page after signing out; otherwise, they are redirected to the sign-in page. This improves user experience by providing a more relevant navigation path based on the server settings.
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughA new configuration property, Changes
Sequence Diagram(s)sequenceDiagram
participant GuestUser
participant Frontend
participant Backend
participant ConfigStore
GuestUser->>Frontend: Access app
Frontend->>Backend: Fetch serverConfig
Backend->>ConfigStore: Read allowGuestDemoWorkspace
ConfigStore-->>Backend: allowGuestDemoWorkspace (true/false)
Backend-->>Frontend: serverConfig (includes allowGuestDemoWorkspace)
alt allowGuestDemoWorkspace is false and user not logged in
Frontend->>GuestUser: Redirect to sign-in page
else allowGuestDemoWorkspace is true or user logged in
Frontend->>GuestUser: Allow access to demo workspace
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🔭 Outside diff range comments (1)
packages/frontend/core/src/modules/cloud/services/servers.ts (1)
76-90: 🛠️ Refactor suggestion
⚠️ Potential issuePersisting servers added before this patch may now return
undefined
serverListStore.addServernow persistsallowGuestDemoWorkspace.
However, existing entries inServerListStorecreated by previous versions will lack this property.
Code that later readsserver.config$.selector(c => c.allowGuestDemoWorkspace)(e.g. navigation guards) may receiveundefinedand mis-behave.Mitigate by normalising on write:
- allowGuestDemoWorkspace: config.allowGuestDemoWorkspace, + allowGuestDemoWorkspace: + config.allowGuestDemoWorkspace ?? true, // fallback for legacy recordsand/or by adding a migration/default on read inside
ServerListStore.
♻️ Duplicate comments (6)
packages/common/graphql/src/graphql/server-config.gql (1)
10-10: Same compatibility caveat as admin querySee remark on
adminServerConfig.gql; apply the same guarded pattern here to avoid runtime 400 errors when hitting an old server.packages/frontend/core/src/modules/cloud/constant.ts (5)
56-57: Same comment as above – duplication only.
81-82: Same comment as above – duplication only.
106-107: Same comment as above – duplication only.
131-132: Same comment as above – duplication only.
156-157: Same comment as above – duplication only.
🧹 Nitpick comments (9)
packages/frontend/core/src/modules/cloud/types.ts (1)
15-18: Interface update looks good but consider documenting default semanticsThe added
allowGuestDemoWorkspaceflag fits the new server contract.
A short JSDoc comment explaining that “true⇒ guests are allowed;false⇒ force sign-in” would improve discoverability for consumers of this type..docker/selfhost/schema.json (1)
590-595: Property addition looks correct, but remember to update public docs / examples.The new
allowGuestDemoWorkspacekey is well-formed (description + default).
Please ensure that any public self-host docs (README, installation guides, and sampleconfig.yaml/envfiles) now mention this flag so operators know it exists; otherwise the feature will remain invisible.packages/frontend/core/src/desktop/pages/invite/index.tsx (1)
24-36: DuplicateuseNavigateHelperhook call
useNavigateHelper()is invoked twice—once to grabjumpToPage(line 24) and again fornavigateHelper(line 35).
Calling the same hook multiple times is legal but wasteful and may allocate two independent helpers.- const { jumpToPage } = useNavigateHelper(); + const navigateHelper = useNavigateHelper(); + const { jumpToPage } = navigateHelper; … - const navigateHelper = useNavigateHelper();Reduces one hook call and simplifies deps arrays.
packages/backend/server/src/core/version/config.ts (1)
32-35: Minor: keep dotted path naming consistentOther keys are nested using dot notation (
versionControl.*).
For consistency considerguestDemoWorkspace.enabledoraccess.allowGuestDemoWorkspace.
Not critical, but keeps the schema tidy and future-proof.packages/frontend/core/src/components/cloud/share-header-right-item/user-avatar.tsx (1)
55-60: Duplicate sign-out logic – favour shared hookThis block repeats the exact flow already added in other components. Extract/ reuse the existing
useSignOut(or create one) to keep behaviour aligned and unit-testable.-const handleSignOut = useAsyncCallback(async () => { - await authService.signOut(); - navigateHelper.jumpToSignIn(); -}, [authService, navigateHelper]); +const handleSignOut = useSignOut();packages/frontend/core/src/modules/cloud/constant.ts (1)
18-31: Avoid repeating identical defaults across every build-target entry
allowGuestDemoWorkspace: trueis duplicated in each server-config block.
Consider defining the common defaults once and spreading them into each variant to keep this table DRY and reduce maintenance overhead when the property list inevitably grows.- config: { + const COMMON_DEFAULTS = { + allowGuestDemoWorkspace: true, + }; + + config: { serverName: 'Affine Selfhost', ... - credentialsRequirement: { ... }, - allowGuestDemoWorkspace: true, + credentialsRequirement: { ... }, + ...COMMON_DEFAULTS, },packages/frontend/core/src/components/hooks/affine/use-sign-out.ts (1)
37-41: Guard against undefined server or stale config before branching
defaultServerService.server.config$.valueassumes bothserverand the reactiveconfig$have been initialised.
A failed service initialisation or a transientnullvalue would currently throw and break the sign-out flow.- if (defaultServerService.server.config$.value.allowGuestDemoWorkspace) { + if (defaultServerService?.server?.config$.value?.allowGuestDemoWorkspace) {Optional chaining gives you a safe fallback path; the else branch already leads to the secure
/sign-inroute.packages/frontend/core/src/desktop/pages/index/index.tsx (2)
92-97: Duplicate guest-redirect logic – extract to helper to stay DRYThe block that wipes
last_workspace_idand callsjumpToSignIn()appears twice (initial navigation effect and workspace-creation effect). Consolidating this into a small utility (or at least auseCallback) will:• Prevent future divergence
• Cut bundle bytes
• Make the intent easier to scan+const redirectUnauthedGuests = useCallback(() => { + localStorage.removeItem('last_workspace_id'); + jumpToSignIn(); +}, [jumpToSignIn]);Then replace the two in-place snippets with
redirectUnauthedGuests();.Also applies to: 145-149
50-50: ConsideruseServiceOptionalfor resiliency
DefaultServerServiceis resolved withuseService, which will throw if the DI container doesn’t provide it (e.g. during unit tests or a stripped-down self-host deployment). UsinguseServiceOptional(and asserting non-null afterwards) avoids a hard crash and aligns with howDesktopApiServiceis handled further down.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.docker/selfhost/schema.json(1 hunks)packages/backend/server/src/core/config/resolver.ts(1 hunks)packages/backend/server/src/core/config/types.ts(1 hunks)packages/backend/server/src/core/version/config.ts(2 hunks)packages/backend/server/src/schema.gql(1 hunks)packages/common/graphql/src/graphql/admin/admin-server-config.gql(1 hunks)packages/common/graphql/src/graphql/index.ts(2 hunks)packages/common/graphql/src/graphql/server-config.gql(1 hunks)packages/common/graphql/src/schema.ts(3 hunks)packages/frontend/admin/src/config.json(1 hunks)packages/frontend/core/src/components/cloud/share-header-right-item/user-avatar.tsx(2 hunks)packages/frontend/core/src/components/hooks/affine/use-sign-out.ts(2 hunks)packages/frontend/core/src/components/workspace-selector/user-with-workspace-list/workspace-list/index.tsx(1 hunks)packages/frontend/core/src/desktop/pages/index/index.tsx(5 hunks)packages/frontend/core/src/desktop/pages/invite/index.tsx(1 hunks)packages/frontend/core/src/mobile/components/workspace-selector/menu.tsx(1 hunks)packages/frontend/core/src/modules/cloud/constant.ts(6 hunks)packages/frontend/core/src/modules/cloud/entities/server.ts(1 hunks)packages/frontend/core/src/modules/cloud/services/servers.ts(1 hunks)packages/frontend/core/src/modules/cloud/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/frontend/core/src/components/hooks/affine/use-sign-out.ts (4)
packages/common/infra/src/framework/react/index.tsx (2)
useService(15-17)useServices(29-45)packages/frontend/core/src/modules/cloud/stores/auth.ts (1)
signOut(103-105)packages/frontend/core/src/modules/cloud/services/auth.ts (1)
signOut(188-192)packages/backend/server/src/base/error/def.ts (1)
UserFriendlyError(63-166)
packages/frontend/core/src/desktop/pages/index/index.tsx (2)
packages/common/infra/src/framework/react/index.tsx (1)
useService(15-17)packages/frontend/core/src/modules/cloud/index.ts (1)
DefaultServerService(12-12)
🔇 Additional comments (11)
packages/backend/server/src/schema.gql (1)
1572-1574:allowGuestDemoWorkspacefield looks correct and is well-documentedThe new non-nullable Boolean is self-explanatory and matches the resolver implementation.
No concerns here.packages/frontend/core/src/mobile/components/workspace-selector/menu.tsx (1)
229-233: Sign-out flow correctly redirects – LGTMThe additional
jumpToSignIn()ensures a consistent post-logout UX, and the dependency array is updated accordingly.packages/frontend/core/src/modules/cloud/entities/server.ts (1)
81-88: Verify store type supports the new field.
allowGuestDemoWorkspaceis forwarded intoupdateServerConfig, but the method’s parameter type (inServerListStore) must include it; otherwise TypeScript will silently allow the excess property at call-site and then drop it, leaving the UI with stale data.// packages/frontend/core/src/modules/cloud/stores/server-list.ts (example) export interface StoredServerConfig { … + allowGuestDemoWorkspace: boolean; }Please confirm the store interface and any persistence layer migrations were updated; otherwise client logic that reads this flag may always fall back to
undefined.packages/frontend/admin/src/config.json (1)
206-210: Consistent “Boolean” type 👍Entry matches the existing typing style in this JSON spec. No issues spotted.
packages/frontend/core/src/desktop/pages/invite/index.tsx (1)
84-88: Good UX improvement – immediate redirect after sign-outAutomatically routing the user to
/sign-ineliminates a blank screen and matches the behaviour introduced elsewhere.packages/common/graphql/src/graphql/admin/admin-server-config.gql (1)
10-10:❓ Verification inconclusive
GraphQL field addition may break older deployments
Adding
allowGuestDemoWorkspaceto the selection set means the query will fail against servers that have not yet been upgraded, blocking the whole UI that depends on this query.
Unless the FE & BE are always deployed atomically, gate the field behind@skip(if:), a separate “probe” query, or use a versioned endpoint.
🏁 Script executed:
#!/bin/bash set -e # Locate the GraphQL file FILE=$(fd admin-server-config.gql -t f) echo "Inspecting file: $FILE" echo "----- File Content (first 200 lines) -----" sed -n '1,200p' "$FILE" # Check where and how this query is used in the codebase echo "----- References to admin-server-config.gql -----" grep -R "admin-server-config.gql" -n packages echo "----- References to query name or import usage -----" grep -R "adminServerConfig" -n packages || trueLength of output: 1047
Gate new GraphQL field in admin-server-config.gql to avoid breaking older deployments
The
allowGuestDemoWorkspacefield was added to the selection set in
packages/common/graphql/src/graphql/admin/admin-server-config.gql (around line 6). Any client querying an older server that doesn’t yet expose this field will receive an error, potentially blocking the UI.Unless your frontend and backend are guaranteed to be deployed atomically, please consider one of the following strategies:
- Wrap
allowGuestDemoWorkspacewith a GraphQL directive, e.g.allowGuestDemoWorkspace @skip(if: $isOldServer)- Extract it into a separate “probe” query that won’t block rendering of the main UI
- Use a versioned GraphQL endpoint so new fields don’t affect older deployments
packages/common/graphql/src/graphql/index.ts (2)
30-36: Field inclusion looks correct
allowGuestDemoWorkspaceis now queried – aligns with schema changes. No issues spotted.
1700-1707: Server-config query updated correctlyThe extra selection ensures clients receive the new flag. Looks good.
packages/common/graphql/src/schema.ts (3)
2153-2154: Expose allowGuestDemoWorkspace in ServerConfigType.This addition correctly extends the GraphQL schema type with the new flag to control guest demo workspace access.
2713-2713: Add allowGuestDemoWorkspace to AdminServerConfigQuery response.Ensures the admin server config query returns the new flag for client-side enforcement.
4657-4657: Include allowGuestDemoWorkspace in ServerConfigQuery response.Makes the standard server config query expose the new flag.
...end/core/src/components/workspace-selector/user-with-workspace-list/workspace-list/index.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12779 +/- ##
==========================================
+ Coverage 55.63% 55.84% +0.20%
==========================================
Files 2653 2654 +1
Lines 125319 125353 +34
Branches 19839 19842 +3
==========================================
+ Hits 69721 69998 +277
+ Misses 53847 53219 -628
- Partials 1751 2136 +385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nhance flexibility in configuration fix(resolver): provide a default value for allowGuestDemoWorkspace to ensure consistent behavior test(useSignOut): add tests for useSignOut hook to verify redirection logic based on guest demo allowance refactor(workspace-list): simplify sign-out handling by using useSignOut hook fix(index): ensure allowGuestDemo defaults to true when not set in configuration to prevent undefined behavior
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/frontend/core/src/components/hooks/affine/__tests__/use-sign-out.spec.ts (1)
1-4: Nit: remove unnecessary ESLint disable
/* eslint-disable rxjs/finnish */appears unrelated to this file (no RxJS).
Unless the linter actually complains, drop the directive to keep the header clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/backend/server/src/core/config/resolver.ts(1 hunks)packages/backend/server/src/core/config/types.ts(1 hunks)packages/backend/server/src/core/version/config.ts(2 hunks)packages/backend/server/src/schema.gql(1 hunks)packages/frontend/core/src/components/hooks/affine/__tests__/use-sign-out.spec.ts(1 hunks)packages/frontend/core/src/components/workspace-selector/user-with-workspace-list/workspace-list/index.tsx(2 hunks)packages/frontend/core/src/desktop/pages/index/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/backend/server/src/core/config/types.ts
- packages/backend/server/src/schema.gql
- packages/backend/server/src/core/config/resolver.ts
- packages/frontend/core/src/components/workspace-selector/user-with-workspace-list/workspace-list/index.tsx
- packages/backend/server/src/core/version/config.ts
- packages/frontend/core/src/desktop/pages/index/index.tsx
packages/frontend/core/src/components/hooks/affine/__tests__/use-sign-out.spec.ts
Outdated
Show resolved
Hide resolved
packages/frontend/core/src/components/hooks/affine/__tests__/use-sign-out.spec.ts
Show resolved
Hide resolved
packages/frontend/core/src/components/hooks/affine/__tests__/use-sign-out.spec.ts
Show resolved
Hide resolved
…r is handled correctly in tests
allowGuestDemoWorkspace-admin-flag.mp4
This PR enforces login‑first access (#8716) by disabling or enabling the guest demo workspace via Admin Server Client Page and redirecting unauthenticated users straight to
/sign‑in.Summary by CodeRabbit