Uh oh!
There was an error while loading. Please reload this page.
chore(web): Remove general settings - #1065
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughRemoves the General settings page, its name/domain change UI and related server actions and schemas; refactors settings layout to use a centralized sidebar helper and adds a settings index page that redirects authenticated users. Analytics and License pages now enforce owner-only access. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/web/src/app/[domain]/settings/page.tsx (1)
14-18: Consider adding a guard for emptyitemsarray.While the current
getSidebarNavItemsimplementation always includes Analytics and License items, accessingitems[0].hrefwithout bounds checking is fragile. Future changes to the sidebar configuration could inadvertently produce an empty array, causing a runtime error.🛡️ Proposed defensive check
const items = await getSidebarNavItems(); if (isServiceError(items)) { throw new ServiceErrorException(items); } + if (items.length === 0) {+ throw new Error("No settings pages available");+ } return redirect(items[0].href);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`[domain]/settings/page.tsx around lines 14 - 18, Add a guard to handle an empty array returned by getSidebarNavItems before dereferencing items[0].href: after calling getSidebarNavItems and checking isServiceError(items), verify items.length > 0 and if not, throw a ServiceErrorException or redirect to a safe default route; update the block that currently does return redirect(items[0].href) to first check items.length and handle the empty case (referencing getSidebarNavItems, isServiceError, ServiceErrorException, and redirect).packages/web/src/app/[domain]/settings/layout.tsx (1)
81-84: MovegetConnectionStats()inside the OWNER conditional block.
connectionStatsis only used in the Connections nav item (line 109), which is gated byrole === OrgRole.OWNER. For MEMBER users, this API call is unnecessary.♻️ Proposed refactor to avoid unnecessary fetch
+ let connectionStats: Awaited<ReturnType<typeof getConnectionStats>> | undefined;+ if (role === OrgRole.OWNER) {- const connectionStats = await getConnectionStats();- if (isServiceError(connectionStats)) {- throw new ServiceErrorException(connectionStats);- }+ connectionStats = await getConnectionStats();+ if (isServiceError(connectionStats)) {+ throw new ServiceErrorException(connectionStats);+ }+ } return [ // ... billing, access, members ... ...(role === OrgRole.OWNER ? [ { title: "Connections", href: `/${SINGLE_TENANT_ORG_DOMAIN}/settings/connections`, hrefRegex: `/${SINGLE_TENANT_ORG_DOMAIN}/settings/connections(/[^/]+)?$`, - isNotificationDotVisible: connectionStats.numberOfConnectionsWithFirstTimeSyncJobsInProgress > 0,+ isNotificationDotVisible: connectionStats!.numberOfConnectionsWithFirstTimeSyncJobsInProgress > 0, } ] : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`[domain]/settings/layout.tsx around lines 81 - 84, Move the getConnectionStats() call and its error handling into the OWNER-only branch so we only fetch when role === OrgRole.OWNER; specifically, remove the top-level const connectionStats = await getConnectionStats() and the subsequent isServiceError(...) / throw new ServiceErrorException(...) and instead call getConnectionStats() inside the conditional that renders or builds the "Connections" nav item (the branch that checks role === OrgRole.OWNER), then run the same isServiceError check and throw ServiceErrorException there so MEMBER users never trigger the API call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/app/`[domain]/settings/layout.tsx:
- Around line 81-84: Move the getConnectionStats() call and its error handling
into the OWNER-only branch so we only fetch when role === OrgRole.OWNER;
specifically, remove the top-level const connectionStats = await
getConnectionStats() and the subsequent isServiceError(...) / throw new
ServiceErrorException(...) and instead call getConnectionStats() inside the
conditional that renders or builds the "Connections" nav item (the branch that
checks role === OrgRole.OWNER), then run the same isServiceError check and throw
ServiceErrorException there so MEMBER users never trigger the API call.
In `@packages/web/src/app/`[domain]/settings/page.tsx:
- Around line 14-18: Add a guard to handle an empty array returned by
getSidebarNavItems before dereferencing items[0].href: after calling
getSidebarNavItems and checking isServiceError(items), verify items.length > 0
and if not, throw a ServiceErrorException or redirect to a safe default route;
update the block that currently does return redirect(items[0].href) to first
check items.length and handle the empty case (referencing getSidebarNavItems,
isServiceError, ServiceErrorException, and redirect).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997c634b-c3d1-4e77-8d92-3ba296cb11d2
📒 Files selected for processing (8)
CHANGELOG.mdpackages/web/src/actions.tspackages/web/src/app/[domain]/settings/(general)/components/changeOrgDomainCard.tsxpackages/web/src/app/[domain]/settings/(general)/components/changeOrgNameCard.tsxpackages/web/src/app/[domain]/settings/(general)/page.tsxpackages/web/src/app/[domain]/settings/layout.tsxpackages/web/src/app/[domain]/settings/page.tsxpackages/web/src/lib/schemas.ts
💤 Files with no reviewable changes (5)
- packages/web/src/app/[domain]/settings/(general)/page.tsx
- packages/web/src/app/[domain]/settings/(general)/components/changeOrgDomainCard.tsx
- packages/web/src/lib/schemas.ts
- packages/web/src/actions.ts
- packages/web/src/app/[domain]/settings/(general)/components/changeOrgNameCard.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit