Feat: enable excel advanced mode on teams by default #1637
Conversation
|
@AshishViradiya153 is attempting to deploy a commit to the mftsio Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce an "Excel Advanced Mode" feature, enabling teams to toggle advanced Excel processing for sheet-type documents. This includes new API endpoints for updating the mode, schema and type updates to persist the setting, UI enhancements for toggling the feature with plan-based gating, and backend logic to handle document processing based on the mode. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant API
participant DB
User->>SettingsPage: Toggle "Excel Advanced Mode"
SettingsPage->>API: PATCH /api/teams/[teamId]/update-advanced-mode
API->>DB: Update Team.enableExcelAdvancedMode
DB-->>API: Success/Failure
API-->>SettingsPage: Response
SettingsPage-->>User: Show toast / modal (if upgrade needed)
sequenceDiagram
participant User
participant UploadPage
participant API
participant DB
participant Storage
User->>UploadPage: Upload sheet document
UploadPage->>API: POST /api/teams/[teamId]/documents
API->>DB: Check Team.enableExcelAdvancedMode
alt Advanced Mode Enabled
API->>Storage: Copy file to bucket server
API->>DB: Update document advanced flags and version
API->>API: Trigger revalidation
else
API->>DB: Standard processing
end
API-->>UploadPage: Response
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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)
lib/documents/create-document.ts (1)
3-48: 💡 Verification agent🧩 Analysis chain
Consider adding the new flag to the request body.
The
enableExcelAdvancedModeproperty has been added to theDocumentDatatype, but it's not included in the request body when creating a document. Consider whether this property should be passed to the API endpoint.
🏁 Script executed:
#!/bin/bash # Search for API endpoint implementations to see if they expect this property rg -A 10 "api/teams/\[teamId\]/documents" --type tsLength of output: 1104
Include
enableExcelAdvancedModein the create‐document flowWe added
enableExcelAdvancedModeto theDocumentDatatype but never send it in the POST or persist it server-side. To fully support this flag:
In lib/documents/create-document.ts, extend the JSON body:
body: JSON.stringify({ name: documentData.name, url: documentData.key, storageType: documentData.storageType, numPages: numPages, folderPathName: folderPathName, type: documentData.supportedFileType, contentType: documentData.contentType, createLink: createLink, fileSize: documentData.fileSize, + enableExcelAdvancedMode: documentData.enableExcelAdvancedMode, }),In your API handler (e.g.
pages/api/teams/[teamId]/documents/index.tsor equivalent), update the POST branch to:
- Destructure
enableExcelAdvancedModefromreq.body.- Persist it on the new document record.
This ensures the client sends the flag and the backend stores it.
🧹 Nitpick comments (1)
pages/api/teams/[teamId]/enable-advanced-mode.ts (1)
40-43: Input payload is not validated
enableExcelAdvancedModeis assumed to be a boolean. A typo ("tru",1, etc.) will silently flip all sheet documents to the opposite state.
Add a runtime guard and early400response:if (typeof enableExcelAdvancedMode !== "boolean") { return res.status(400).json({ error: "`enableExcelAdvancedMode` must be a boolean" }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (15)
app/api/links/[id]/upload/route.ts(2 hunks)components/documents/document-header.tsx(1 hunks)components/ui/form.tsx(6 hunks)context/team-context.tsx(2 hunks)lib/api/documents/process-document.ts(4 hunks)lib/documents/create-document.ts(1 hunks)lib/types.ts(1 hunks)pages/api/teams/[teamId]/documents/index.ts(1 hunks)pages/api/teams/[teamId]/enable-advanced-mode.ts(1 hunks)pages/api/teams/[teamId]/update-advanced-mode.ts(1 hunks)pages/api/teams/[teamId]/update-name.ts(2 hunks)pages/api/teams/index.ts(2 hunks)pages/settings/general.tsx(2 hunks)prisma/migrations/20250516000000_add_advanced_mode_to_team/migration.sql(1 hunks)prisma/schema/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/api/links/[id]/upload/route.ts (1)
lib/api/documents/process-document.ts (1)
processDocument(26-246)
context/team-context.tsx (1)
lib/types.ts (1)
Team(296-303)
pages/api/teams/[teamId]/update-advanced-mode.ts (4)
pages/api/teams/[teamId]/enable-advanced-mode.ts (1)
handle(21-135)pages/api/teams/[teamId]/update-name.ts (1)
handle(10-68)pages/api/auth/[...nextauth].ts (1)
authOptions(25-189)lib/types.ts (1)
CustomUser(18-18)
🔇 Additional comments (28)
lib/types.ts (1)
302-302: LGTM: New team property added correctly.Adding an optional boolean property to support Excel Advanced Mode at the team level makes sense. This property aligns with the database schema changes in the migration file.
lib/documents/create-document.ts (1)
11-11: LGTM: New document property added correctly.The optional
enableExcelAdvancedModeproperty has been added to support advanced Excel features for sheet-type documents. This aligns with the team-level setting and enables proper document processing based on this mode.prisma/migrations/20250516000000_add_advanced_mode_to_team/migration.sql (1)
1-3: LGTM: Database migration is correctly implemented.The migration adds a non-nullable boolean column with a default value of
false, which ensures backward compatibility for existing teams. This aligns with the type changes in the codebase.pages/api/teams/[teamId]/update-name.ts (2)
15-15: Corrected API method comment.Fixed comment to accurately reflect that this is a PATCH endpoint rather than POST.
64-66: Fixed allowed methods header.Corrected the Allow header to properly indicate PATCH is the only allowed method, which is consistent with the implementation.
prisma/schema/schema.prisma (1)
103-104: LGTM! Team setting for Excel advanced mode added correctly.The new boolean field
enableExcelAdvancedModeis properly added to theTeammodel with an appropriate default value offalse. The comment clearly explains the purpose of this setting.pages/api/teams/[teamId]/documents/index.ts (1)
281-283: LGTM! Excel advanced mode correctly conditionally enabled.The implementation properly passes the team's Excel advanced mode setting to the document processing function, but only when the document is actually a sheet-type document. This conditional check ensures the setting is only applied to relevant documents.
app/api/links/[id]/upload/route.ts (3)
54-54: LGTM! Team setting selection added.Correctly fetches the
enableExcelAdvancedModeflag from the team record when querying the link.
91-95: LGTM! Excel advanced mode correctly propagated to document data.The implementation creates a new object with all existing document data and adds the
enableExcelAdvancedModeproperty from the team settings. The fallback tofalseis appropriate when the team setting isn't available.
99-100: LGTM! Updated document data passed to process function.The updated document data with the Excel advanced mode flag is correctly passed to the
processDocumentfunction.pages/api/teams/index.ts (2)
37-37: LGTM! Team setting included in user teams query.The
enableExcelAdvancedModefield is correctly included in the team data selection when fetching a user's teams.
70-70: LGTM! Team setting included in default team creation.The
enableExcelAdvancedModefield is correctly included in the selection when creating and returning a default team for users with no teams.context/team-context.tsx (2)
19-19: LGTM: Adding currentTeamId to TeamContextTypeThis properly extends the context type to include the current team ID as a direct property.
27-27: LGTM: Setting appropriate initial stateThe initial state correctly sets currentTeamId to null, matching the type definition.
components/documents/document-header.tsx (1)
289-312: Improved error handling with toast.promiseThe refactoring to use
toast.promisecreates more consistent error handling and reduces code duplication. This is a good improvement that aligns with how other API calls are handled in this file.components/ui/form.tsx (7)
14-15: LGTM: Importing PlanBadge for plan-based feature gatingThis import supports showing plan badges in the form for plan-restricted features.
25-26: LGTM: Adding Label and Switch importsThese imports are needed for the new checkbox input type implementation.
38-39: LGTM: Adding plan prop to Form componentThis properly extends the Form component to support displaying plan badges with form fields.
Also applies to: 49-50
58-66: LGTM: Updated saveDisabled logic for checkbox inputsThe saveDisabled logic now correctly handles checkbox input types by comparing boolean values.
68-109: LGTM: Added renderInput functionThe new renderInput function elegantly handles conditional rendering of different input types.
129-131: LGTM: Updated CardTitle to show plan badgeThe CardTitle now conditionally displays a PlanBadge when the plan prop is provided.
136-137: LGTM: Using renderInputReplaced inline JSX with the new renderInput function call for better code organization.
lib/api/documents/process-document.ts (3)
4-4: LGTM: Importing copyFileToBucketServerThis import is necessary for the new advanced Excel mode functionality.
43-44: LGTM: Extracting enableExcelAdvancedMode from documentDataProperly destructuring the new flag from the DocumentData parameter.
90-91: LGTM: Setting advancedExcelEnabled in document creationThe document creation properly persists the Excel advanced mode setting.
pages/api/teams/[teamId]/enable-advanced-mode.ts (1)
80-88: Primary version existence is not guaranteed
const primaryVersion = doc.versions[0];can beundefined(e.g., a sheet without a primary version).
When that happens, the document is skipped entirely, leaving itsadvancedExcelEnabledflag inconsistent with the team setting.At minimum log / surface this case or filter the initial query to ensure
versionsis non-empty:where: { teamId: teamId as string, type: "sheet", advancedExcelEnabled: !enableExcelAdvancedMode, + versions: { some: { isPrimary: true, type: "sheet" } }, // guarantees at least one },pages/settings/general.tsx (2)
45-60: Error-handling assumes{ error: { message } }– breaks on string responses
/api/teams/[teamId]/update-advanced-modecurrently returns a string on success and possibly on failure.
await res.json()will parse correctly, but destructuring{ error }and then readingerror.messagethrows whenerroris a plain string.Defensive fix:
- const { error } = await res.json(); - throw new Error(error.message); + const payload = await res.json(); + if (typeof payload === "string") { + throw new Error(payload); + } + throw new Error(payload?.error ?? "Unknown server error");
32-38: Double gating: move “free-plan” check to UI layer onlyYou already disable the toggle visually via the
planprop and show the upgrade modal.
Keeping this early-return here is fine, but remember that the backend still must enforce plan limits; otherwise a crafted request enables the feature. Verify the server route mirrors this logic (see previous comment).
| currentTeam: | ||
| (teams || []).find((team) => team.id === currentTeamId) || | ||
| (teams || [])[0], | ||
| currentTeamId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add currentTeamId to useMemo dependencies
The context value now includes currentTeamId, but this value is not included in the dependency array on line 58.
const value = useMemo(
() => ({
teams: teams || [],
currentTeam:
(teams || []).find((team) => team.id === currentTeamId) ||
(teams || [])[0],
currentTeamId,
isLoading: loading,
setCurrentTeam,
}),
- [teams, currentTeam, loading],
+ [teams, currentTeam, loading, currentTeamId, setCurrentTeam],
);📝 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.
| currentTeamId, | |
| const value = useMemo( | |
| () => ({ | |
| teams: teams || [], | |
| currentTeam: | |
| (teams || []).find((team) => team.id === currentTeamId) || | |
| (teams || [])[0], | |
| currentTeamId, | |
| isLoading: loading, | |
| setCurrentTeam, | |
| }), | |
| [teams, currentTeam, loading, currentTeamId, setCurrentTeam], | |
| ); |
🤖 Prompt for AI Agents
In context/team-context.tsx at line 54, the currentTeamId is included in the
context value but is missing from the useMemo dependency array around line 58.
Add currentTeamId to the dependency array of the useMemo hook to ensure the
memoized value updates correctly when currentTeamId changes.
| if (type === "sheet" && enableExcelAdvancedMode) { | ||
| await copyFileToBucketServer({ | ||
| filePath: document.versions[0].file, | ||
| storageType: document.versions[0].storageType, | ||
| }); | ||
|
|
||
| await prisma.documentVersion.update({ | ||
| where: { id: document.versions[0].id }, | ||
| data: { numPages: 1 }, | ||
| }); | ||
|
|
||
| try { | ||
| await fetch( | ||
| `${process.env.NEXTAUTH_URL}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${document.id}`, | ||
| ); | ||
| } catch (error) { | ||
| console.error("Failed to revalidate document:", error); | ||
| // The document is still updated, so we can continue without throwing | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add environment variable validation for revalidation URL
The implementation for the Excel advanced mode looks good overall, but there's a potential issue with the revalidation URL. The code uses process.env.NEXTAUTH_URL without checking if it's defined.
if (type === "sheet" && enableExcelAdvancedMode) {
await copyFileToBucketServer({
filePath: document.versions[0].file,
storageType: document.versions[0].storageType,
});
await prisma.documentVersion.update({
where: { id: document.versions[0].id },
data: { numPages: 1 },
});
try {
+ const revalidateUrl = process.env.NEXTAUTH_URL;
+ if (!revalidateUrl) {
+ console.error("NEXTAUTH_URL is not defined, skipping revalidation");
+ return document;
+ }
await fetch(
- `${process.env.NEXTAUTH_URL}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${document.id}`,
+ `${revalidateUrl}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${document.id}`,
);
} catch (error) {
console.error("Failed to revalidate document:", error);
// The document is still updated, so we can continue without throwing
}
}📝 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.
| if (type === "sheet" && enableExcelAdvancedMode) { | |
| await copyFileToBucketServer({ | |
| filePath: document.versions[0].file, | |
| storageType: document.versions[0].storageType, | |
| }); | |
| await prisma.documentVersion.update({ | |
| where: { id: document.versions[0].id }, | |
| data: { numPages: 1 }, | |
| }); | |
| try { | |
| await fetch( | |
| `${process.env.NEXTAUTH_URL}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${document.id}`, | |
| ); | |
| } catch (error) { | |
| console.error("Failed to revalidate document:", error); | |
| // The document is still updated, so we can continue without throwing | |
| } | |
| } | |
| if (type === "sheet" && enableExcelAdvancedMode) { | |
| await copyFileToBucketServer({ | |
| filePath: document.versions[0].file, | |
| storageType: document.versions[0].storageType, | |
| }); | |
| await prisma.documentVersion.update({ | |
| where: { id: document.versions[0].id }, | |
| data: { numPages: 1 }, | |
| }); | |
| try { | |
| const revalidateUrl = process.env.NEXTAUTH_URL; | |
| if (!revalidateUrl) { | |
| console.error("NEXTAUTH_URL is not defined, skipping revalidation"); | |
| return document; | |
| } | |
| await fetch( | |
| `${revalidateUrl}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${document.id}`, | |
| ); | |
| } catch (error) { | |
| console.error("Failed to revalidate document:", error); | |
| // The document is still updated, so we can continue without throwing | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/api/documents/process-document.ts around lines 205 to 224, the code uses
process.env.NEXTAUTH_URL directly in the revalidation URL without verifying if
it is defined, which can cause runtime errors. Add a check to ensure
process.env.NEXTAUTH_URL is defined before using it in the fetch call. If it is
undefined, handle the case gracefully by either skipping the fetch or logging a
warning, to prevent potential failures.
| const result = await prisma.$transaction(async (tx) => { | ||
| // Get all Excel documents in the team that need to be updated | ||
| const documents = await tx.document.findMany({ | ||
| where: { | ||
| teamId: teamId as string, | ||
| type: "sheet", | ||
| advancedExcelEnabled: !enableExcelAdvancedMode, | ||
| }, | ||
| include: { | ||
| versions: { | ||
| where: { | ||
| isPrimary: true, | ||
| type: "sheet", | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // Update all documents and their versions | ||
| const updatePromises = documents.map(async (doc) => { | ||
| const primaryVersion = doc.versions[0]; | ||
| if (!primaryVersion) return; | ||
|
|
||
| if (enableExcelAdvancedMode) { | ||
| // Copy file to bucket if enabling advanced mode | ||
| await copyFileToBucketServer({ | ||
| filePath: primaryVersion.file, | ||
| storageType: primaryVersion.storageType, | ||
| }); | ||
|
|
||
| // Update document and version when enabling | ||
| await Promise.all([ | ||
| tx.document.update({ | ||
| where: { id: doc.id }, | ||
| data: { advancedExcelEnabled: true }, | ||
| }), | ||
| tx.documentVersion.update({ | ||
| where: { id: primaryVersion.id }, | ||
| data: { numPages: 1 }, | ||
| }), | ||
| ]); | ||
| } else { | ||
| await tx.document.update({ | ||
| where: { id: doc.id }, | ||
| data: { advancedExcelEnabled: false }, | ||
| }); | ||
| } | ||
|
|
||
| // Revalidate the document | ||
| await fetch( | ||
| `${process.env.NEXTAUTH_URL}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${doc.id}`, | ||
| ); | ||
| }); | ||
|
|
||
| await Promise.all(updatePromises); | ||
| const updatedTeam = await tx.team.update({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-DB side-effects inside a Prisma transaction
copyFileToBucketServer (network / storage I/O) and the fetch(.../revalidate) call are executed inside the $transaction closure.
Because they are not part of the database, two issues arise:
- If one of those side-effects succeeds but a later DB query fails, the SQL transaction will roll back while the external side-effects remain → inconsistent state (orphaned files, premature cache re-validation).
- The long-running I/O keeps the DB transaction open, increasing lock-time and contention risk.
Consider this safer flow:
// 1. Gather ids / versions that need work ➜ tx only
- const result = await prisma.$transaction(async (tx) => {
+ const {team, affectedDocs} = await prisma.$transaction(async (tx) => {
...
- // do NOT call copyFileToBucketServer or fetch here
return { team: updatedTeam, affectedDocs: documents.map(d => ({ id: d.id, file: d.versions[0]?.file, storageType: d.versions[0]?.storageType })) }
});
+// 2. Perform external work after commit
+await Promise.all(
+ affectedDocs.map(async (d) => {
+ if (enableExcelAdvancedMode && d.file) {
+ await copyFileToBucketServer({ filePath: d.file, storageType: d.storageType });
+ }
+ await fetch(`${process.env.NEXTAUTH_URL}/api/revalidate?secret=${process.env.REVALIDATE_TOKEN}&documentId=${d.id}`);
+ })
+);This guarantees DB integrity and keeps the SQL lock window minimal.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/api/teams/[teamId]/enable-advanced-mode.ts between lines 60 and 115,
the calls to copyFileToBucketServer and fetch for revalidation are inside the
Prisma $transaction, causing side-effects during the DB transaction. To fix
this, move all non-database side-effect operations like file copying and HTTP
fetch calls outside and after the $transaction block. First, perform all
database updates within the transaction, then sequentially execute the external
side-effects to avoid inconsistent states and reduce DB lock duration.
| if (!team || team.plan.includes("free")) { | ||
| return res.status(401).json({ error: "Unauthorized" }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Plan check may throw when plan is null and is case-sensitive
team.plan.includes("free"):
- Throws if
planisnull. - Fails silently if plans are stored as
"Free"/ enum values.
Prefer a strict comparison against your existing PlanEnum:
-import { PlanEnum } from "@/ee/stripe/constants";
...
-if (!team || team.plan.includes("free")) {
+if (!team || team.plan === PlanEnum.Free) {
return res.status(401).json({ error: "Unauthorized" });
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/api/teams/[teamId]/update-advanced-mode.ts around lines 45 to 47, the
current check team.plan.includes("free") can throw an error if plan is null and
is case-sensitive, which may cause incorrect authorization logic. To fix this,
first ensure plan is not null before checking its value, and replace the
includes check with a strict comparison against the PlanEnum values, handling
case sensitivity properly. This will prevent runtime errors and ensure accurate
plan validation.
| const team = await prisma.team.findUnique({ | ||
| where: { | ||
| id: teamId, | ||
| users: { | ||
| some: { | ||
| userId: (session.user as CustomUser).id, | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
findUnique with relation filters is invalid – compilation will fail
findUnique only supports unique scalar fields. Passing users: { some: ... } will raise a Prisma type error.
- const team = await prisma.team.findUnique({
+ const team = await prisma.team.findFirst({
where: {
id: teamId,
users: {
some: {
userId: (session.user as CustomUser).id,
},
},
},
});📝 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.
| const team = await prisma.team.findUnique({ | |
| where: { | |
| id: teamId, | |
| users: { | |
| some: { | |
| userId: (session.user as CustomUser).id, | |
| }, | |
| }, | |
| }, | |
| }); | |
| const team = await prisma.team.findFirst({ | |
| where: { | |
| id: teamId, | |
| users: { | |
| some: { | |
| userId: (session.user as CustomUser).id, | |
| }, | |
| }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In pages/api/teams/[teamId]/update-advanced-mode.ts around lines 35 to 44, the
use of findUnique with a relation filter on users is invalid and causes a
compilation error. Replace findUnique with findFirst to allow filtering by
related users. Adjust the query to use findFirst with the same where clause
including the users.some condition to correctly filter teams by teamId and user
membership.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores