todo: 2nd PR for fixing and updating leftover things#10
todo: 2nd PR for fixing and updating leftover things#10pantharshit007 merged 10 commits intomasterfrom
Conversation
- Introduced a centralized error handling utility in `convex/lib/errors.ts` to standardize error throwing and logging. - Replaced direct `ConvexError` throws with the new `throwError` function in various files including `convex/projects.ts`, `convex/sharedSecrets.ts`, `convex/users.ts`, and `convex/variables.ts`. - Updated frontend components to utilize `getErrorMessage` for improved error messaging, enhancing user feedback in components such as `EnvironmentVariables`, `MembersDrawer`, and `ProjectMembers`. - Ensured all error messages are structured and provide context for better debugging and user experience.
…hance error handling and UI updates
- Implemented an alert dialog to confirm deletion of environment variables. - Replaced the previous confirm prompt with a more user-friendly dialog. - Added state management for tracking the variable to be deleted. - Enhanced user experience by providing clear feedback on the deletion action.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR centralizes backend error handling with a typed throwError utility and numeric codes, renames platformRole → tier across backend and frontend, adds environment.updatedBy and updater metadata, implements paginated + bulk operations for shared secrets (with UI select-all), and applies input length validation and centralized client error extraction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as rgba(60,120,255,0.5) Client (browser)
participant UI as rgba(20,180,120,0.5) Frontend UI
participant API as rgba(200,100,40,0.5) Convex backend (convex/sharedSecrets)
participant DB as rgba(160,40,180,0.5) Database
Client->>UI: Click "Load shared secrets"
UI->>API: usePaginatedQuery(paginatedListByUser, paginationOpts)
API->>DB: query sharedSecrets + join projects/environments/users (page)
DB-->>API: return rows + counts
API-->>UI: paginated items + canLoadMore
UI-->>Client: render page, show checkboxes and Bulk Actions
Client->>UI: Select multiple items -> "Delete selected"
UI->>API: mutation bulkRemove({ ids })
API->>DB: for each id: check access, delete, adjust quotas
DB-->>API: deleted rows, updated quotas
API-->>UI: success / errors (stripped payload)
UI-->>Client: show toast, refresh page or update items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/users.ts (2)
18-33:⚠️ Potential issue | 🟡 MinorValidate the derived display name too.
When
args.nameis omitted, the fallback from email can exceed the max length; validate the computednameto avoid overlong values.Suggested fix
- validateLength(args.name, 50, "Name"); validateLength(args.email, 100, "Email"); const name = (args.name || args.email.split("@")[0] || "Unknown User").trim(); + validateLength(name, 50, "Name");
139-148:⚠️ Potential issue | 🟡 MinorDefault tier to "free" for legacy users.
If older records lack
tier, returningundefinedcan ripple into UI/limit logic; defaulting keeps behavior consistent.Suggested fix
- tier: user.tier, + tier: user.tier || "free",src/lib/types.ts (1)
3-13:⚠️ Potential issue | 🟡 MinorMake
updaterNameandupdaterImageoptional in the type definition.The query handler explicitly allows these fields to be
undefined(lines 52–57 in convex/environments.ts) when the user record is not found, but the type definition declares them as required strings. This type mismatch can mask runtimeundefinedin components that use these fields without null checks.Suggested fix
- updatedBy: Id<"users">; - updaterName: string; - updaterImage: string; + updatedBy: Id<"users">; + updaterName?: string; + updaterImage?: string;
🤖 Fix all issues with AI agents
In `@convex/lib/roleLimits.ts`:
- Around line 63-65: The code returns TIER_LIMITS[tier] directly which can be
undefined for legacy or unknown tier strings (e.g., "user"); update
getTierLimits to defensively normalize and validate the input (map legacy names
to current Tier values and fall back to "free") and always return a valid
TierLimits object; replace other direct index usages of TIER_LIMITS (the other
occurrences referenced) to call getTierLimits so callers never receive undefined
limits and preserve the return type.
In `@convex/projects.ts`:
- Around line 89-91: The create path currently validates args.passcodeHint with
validateLength(..., 75, "Passcode hint") while updateProject uses 100; make them
consistent by updating the create function's validation to
validateLength(args.passcodeHint, 100, "Passcode hint") (or alternatively change
updateProject to 75) so both create and updateProject use the same limit; locate
the calls to validateLength in the create function and the updateProject
function and align the numeric limit for passcodeHint.
In `@convex/schema.ts`:
- Line 52: The schema declares updatedBy as required (updatedBy: v.id("users"))
but code in convex/environments.ts checks for env.updatedBy, so either make
updatedBy optional by changing the schema to use v.optional(v.id("users")) (and
update any validation/tests) or add a one-off migration that backfills existing
environment documents with a valid users id or a null/placeholder value so the
current required schema is satisfied; update the symbol updatedBy and ensure
convex/environments.ts behavior aligns with the chosen approach (optional field
handling vs. guaranteed presence after migration).
In `@convex/sharedSecrets.ts`:
- Around line 522-548: In bulkUpdateExpiry's handler ensure a runtime validation
that when args.isIndefinite is false an expiresAt is provided: at the top of the
handler (before looping and calling
checkSecretManagementAccess/getProjectOwnerLimits) throwError if
args.isIndefinite === false && args.expiresAt == null so you don't create
invalid records; keep the schema as-is but enforce this check and return a clear
error message and code consistent with other checks.
- Around line 1-8: The project version in package.json wasn't incremented for
this feature release (bulk actions + pagination); update the "version" field in
package.json to the appropriate next semantic version (e.g., 0.1.0 for a minor
feature release or 0.0.10 for a patch) and commit that change so consumers see
the release; you can locate the change context near imports used by this feature
(symbols like paginationOptsValidator, mutation, query, and functions
checkAndClearPlanEnforcementFlag/getProjectOwnerLimits referenced in
convext/sharedSecrets.ts) to ensure the version bump aligns with the released
code.
In `@docs/learning.md`:
- Around line 794-799: The markdown table header separator row lacks spaces
around pipes; update the table that maps
platformRole/PlatformRole/ROLE_LIMITS/RoleLimits to
tier/Tier/TIER_LIMITS/TierLimits by adding single spaces around each pipe in the
separator (e.g., change "|----------|----------|" to "| ---------- | ----------
|") and ensure the header and data rows also have single spaces around pipes to
satisfy MD060.
In `@src/components/environment-variables/EnvironmentVariables.tsx`:
- Around line 17-25: The code indexes TIER_LIMITS using the incoming ownerTier
prop which can contain unexpected strings and thus fall through to showing "∞";
normalize ownerTier before lookup by mapping/validating it to a known Tier
(e.g., coerce or validate into one of the Tier union values and default to
"free" on miss) wherever TIER_LIMITS is accessed (references:
EnvironmentVariables component usages around the initial import area, the lookup
sites originally at lines ~64-69, and ~372-375). Implement a small helper like
normalizeTier(ownerTier) and use its return when doing TIER_LIMITS[...], or
tighten the prop type to the Tier union so the lookup cannot receive invalid
values.
- Around line 378-389: Guard against missing updater metadata by rendering a
fallback avatar and tooltip when environment.updaterImage or
environment.updaterName are absent: in the JSX block that currently renders the
<img> using environment.updaterImage and the tooltip using
environment.updaterName, conditionally render either (a) a default avatar (e.g.,
initials or placeholder SVG) when environment.updaterImage is falsy and (b) a
default tooltip text like "Unknown" when environment.updaterName is falsy;
ensure the wrapper (the div with className="relative group") still exists so the
tooltip positioning works and reference environment.updaterImage and
environment.updaterName in the conditional logic to locate the exact spot to
change.
In `@src/lib/time.ts`:
- Around line 5-30: formatRelativeTime currently returns "just now" for
timestamps within 60 seconds regardless of future vs past; change the seconds
branch to handle future timestamps explicitly (use isFuture to return either "in
{seconds}s" or a human-friendly "in a few seconds") and keep past as "just now";
update the seconds branch in formatRelativeTime and reuse getFormat or inline
`in ${seconds}s` as appropriate so future sub-60s times read correctly (refer to
function formatRelativeTime and helper getFormat).
🧹 Nitpick comments (14)
src/components/environment-variables/VariableEditRow.tsx (1)
25-36: Add an explicit return type for the exported component.It improves signature clarity under strict TS.
As per coding guidelines, Prefer explicit return types for exported functions.♻️ Suggested update
-export function VariableEditRow({ +export function VariableEditRow({ name, value, onNameChange, onValueChange, onSave, onCancel, isSaving, isNew = false, onDelete, onBulkPaste, -}: VariableEditRowProps) { +}: VariableEditRowProps): JSX.Element {convex/lib/errors.ts (1)
54-57: Add an explicit return type for the exported utility.This keeps exported APIs precise under strict TS.
As per coding guidelines, Prefer explicit return types for exported functions.♻️ Suggested update
-export function validateLength(val: string | undefined, max: number, fieldName: string) { +export function validateLength(val: string | undefined, max: number, fieldName: string): void { if (val && val.length > max) { throwError(`${fieldName} is too long (max ${max} characters)`, "BAD_REQUEST", 400); } }src/components/share-dialog.tsx (1)
56-63: Add an explicit return type for the exported component.Keeps the public API precise under strict TS.
As per coding guidelines, Prefer explicit return types for exported functions.♻️ Suggested update
-export function ShareDialog({ +export function ShareDialog({ variables, environment, derivedKey, createShare, trigger, -}: ShareDialogProps) { +}: ShareDialogProps): JSX.Element {src/lib/types.ts (1)
73-78: Define aTiertype and tightenownerTierto use it.The code already casts
ownerTier as Tieron line 374 of EnvironmentVariables.tsx but theTiertype is undefined. Defining it prevents invalid tier values from flowing through the UI.Suggested refactor
+export type Tier = "free" | "pro" | "pro_plus" | "super_admin"; + export interface EnvironmentVariablesProps { environment: Environment; derivedKey: CryptoKey | null; userRole: "owner" | "admin" | "member"; - ownerTier: string; + ownerTier: Tier; }convex/admin.ts (1)
122-133: Consider adding explicit type annotation forcurrentTier.While
targetUser.tieris typed via the schema, adding an explicit type annotation improves readability and catches potential issues if the schema changes.💡 Suggested improvement
- const currentTier = targetUser.tier; - const newTier = args.tier; + const currentTier: "free" | "pro" | "pro_plus" | "super_admin" = targetUser.tier; + const newTier: "free" | "pro" | "pro_plus" | "super_admin" = args.tier;Alternatively, import and use the
Tiertype from./lib/roleLimits:+import type { Tier } from "./lib/roleLimits"; // ... - const currentTier = targetUser.tier; + const currentTier: Tier = targetUser.tier;src/components/app-sidebar.tsx (1)
79-83: Consider simplifying the tier fallback logic.The tier fallback to
"free"is applied twice: once on line 79 and again on line 83. This redundancy is harmless but could be simplified.💡 Minor simplification
const user = clerkUser ? { name: clerkUser.fullName || clerkUser.username || "User", email: clerkUser.primaryEmailAddress?.emailAddress || "", image: clerkUser.imageUrl || null, tier: (me?.tier as Tier) || "free", } : null; - const limits = me ? TIER_LIMITS[user?.tier || "free"] : null; + const limits = user ? TIER_LIMITS[user.tier] : null;Since
user.tieralready has the"free"fallback applied, andlimitsshould only be computed whenuserexists, this simplifies the logic.src/components/environment-variables/VariableRow.tsx (2)
91-101: Addtitleattribute to Button component.The reveal/hide Button is missing a
titleattribute, which is required per coding guidelines for accessibility and UX consistency.Proposed fix
<Button variant="ghost" size="icon" className="h-8 w-8 hover:bg-accent shrink-0" disabled={!derivedKey} + title={isRevealed ? "Hide value" : "Click to reveal"} onClick={() => onReveal(variable._id, variable.encryptedValue, variable.iv, variable.authTag) } >As per coding guidelines:
src/**/*.tsx: Add title attribute to Button components from@/components/ui/button.
151-157: Addtitleattribute to the dropdown trigger Button.Proposed fix
<Button variant="ghost" size="icon" className="h-8 w-8 hover:bg-accent transition-opacity" + title="More actions" >As per coding guidelines:
src/**/*.tsx: Add title attribute to Button components from@/components/ui/button.src/routes/d/admin.tsx (2)
76-85: Avoidanytypes; use explicit types for strict TypeScript compliance.The
userIdandtierparameters useany, which bypasses TypeScript's type checking. Based on the mutation signature inconvex/admin.ts, these should be properly typed.Proposed fix
+import type { Id } from "../../../convex/_generated/dataModel"; +type UserTier = "free" | "pro" | "pro_plus" | "super_admin"; - const handleUpdateTier = async (userId: any, tier: any) => { + const handleUpdateTier = async (userId: Id<"users">, tier: UserTier) => {As per coding guidelines:
**/*.{ts,tsx}: Strict TypeScript mode enabled - No implicit any.
213-228: Fix indentation inconsistency.The opening
<div>tag on line 213 has inconsistent indentation compared to the surrounding code structure.Proposed fix
<CardContent> - <div className="space-y-4"> + <div className="space-y-4"> {Object.entries(metrics.tierDistribution).map(([tier, count]) => (convex/environments.ts (1)
30-30: Consider breaking long line for readability.This line exceeds the 100 character print width guideline.
Proposed fix
- if (!membership) throwError("Access denied", "FORBIDDEN", 403, { user_id: user._id, project_id: projectId }); + if (!membership) { + throwError("Access denied", "FORBIDDEN", 403, { + user_id: user._id, + project_id: projectId, + }); + }As per coding guidelines:
**/*.{ts,tsx,js,jsx,json,css}: Use 100 character print width for line length.src/routes/d/shared.tsx (3)
149-157: Avoid returning values fromforEachcallback.The static analysis tool flags that
next.delete(s._id)returns a boolean value from theforEachcallback. While this doesn't cause a bug (the return value is ignored), it's cleaner to be explicit.Proposed fix
function handleSelectAll(checked: boolean) { if (checked) { setSelectedIds(new Set([...selectedIds, ...filteredShares.map((s) => s._id)])); } else { const next = new Set(selectedIds); - filteredShares.forEach((s) => next.delete(s._id)); + filteredShares.forEach((s) => { next.delete(s._id); }); setSelectedIds(next); } }
181-184: Useunknowninstead ofanyfor error types.The catch blocks use
err: anywhich bypasses TypeScript's type checking. Since you're already usinggetErrorMessagewhich handlesunknown, this should be typed asunknown.Proposed fix
- } catch (err: any) { + } catch (err: unknown) { console.error("Bulk delete failed:", err); toast.error(getErrorMessage(err, "Failed to delete links")); }- } catch (err: any) { + } catch (err: unknown) { console.error("Bulk toggle failed:", err); toast.error(getErrorMessage(err, `Failed to ${isDisabled ? "disable" : "enable"} links`)); }As per coding guidelines:
**/*.{ts,tsx}: Strict TypeScript mode enabled - No implicit any.Also applies to: 197-200
168-185: Consider using AlertDialog for bulk delete confirmation.The PR objectives mention implementing a delete confirmation dialog (AlertDialog) for environment variables. For consistency, the bulk delete here still uses the native
confirm(). Consider using the same AlertDialog pattern for a unified UX.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .withIndex("by_tokenIdentifier", (q) => q.eq("tokenIdentifier", identity.tokenIdentifier)) | ||
| .unique(); | ||
|
|
||
| if (!user) { | ||
| throw new ConvexError("User not found"); | ||
| if (!user || user.isDeactivated) { | ||
| return { page: [], isDone: true, continueCursor: "" }; | ||
| } | ||
|
|
||
| if (user.isDeactivated) { | ||
| throw new ConvexError("User account is deactivated"); | ||
| } | ||
| // Get ALL memberships to identify projects the user has access to | ||
| const allMemberships = await ctx.db | ||
| .query("projectMembers") | ||
| .withIndex("by_userId", (q) => q.eq("userId", user._id)) | ||
| .collect(); | ||
|
|
||
| const sharedSecret = await ctx.db.get(args.id); | ||
| if (!sharedSecret) { | ||
| throw new ConvexError("Shared secret not found"); | ||
| const roleMap = new Map<string, string>(); | ||
| const ownerProjectIds: Id<"projects">[] = []; | ||
| for (const m of allMemberships) { | ||
| roleMap.set(m.projectId, m.role); | ||
| if (m.role === "owner") { | ||
| ownerProjectIds.push(m.projectId); | ||
| } | ||
| } | ||
|
|
||
| // Check user's current role in the project | ||
| const membership = await ctx.db | ||
| .query("projectMembers") | ||
| .withIndex("by_project_user", (q) => | ||
| q.eq("projectId", sharedSecret.projectId).eq("userId", user._id) | ||
| ) | ||
| .unique(); | ||
| // Fetch secrets: created by user OR in owned projects | ||
| const [userSharedSecrets, ...ownerProjectSecrets] = await Promise.all([ | ||
| ctx.db | ||
| .query("sharedSecrets") | ||
| .withIndex("by_createdBy", (q) => q.eq("createdBy", user._id)) | ||
| .collect(), | ||
| ...ownerProjectIds.map((projectId) => | ||
| ctx.db | ||
| .query("sharedSecrets") | ||
| .withIndex("by_projectId", (q) => q.eq("projectId", projectId)) | ||
| .collect() | ||
| ), | ||
| ]); | ||
|
|
||
| if (!membership) { | ||
| throw new ConvexError("Access denied - not a project member"); | ||
| } | ||
| // Deduplicate and combine | ||
| const userSecretIds = new Set(userSharedSecrets.map((s) => s._id)); | ||
| const ownerSharedSecrets = ownerProjectSecrets | ||
| .flat() | ||
| .filter((secret) => !userSecretIds.has(secret._id)); | ||
|
|
||
| const allSharedSecrets = [...userSharedSecrets, ...ownerSharedSecrets].sort( | ||
| (a, b) => b._creationTime - a._creationTime | ||
| ); | ||
|
|
||
| // Allow if: owner of project, OR creator who is still admin/owner | ||
| const isOwner = membership.role === "owner"; | ||
| const isCreatorWithAdminAccess = | ||
| sharedSecret.createdBy === user._id && | ||
| (membership.role === "owner" || membership.role === "admin"); | ||
| // Manual pagination | ||
| const { numItems, cursor } = args.paginationOpts; | ||
| const startIndex = cursor ? parseInt(cursor, 10) : 0; | ||
| const endIndex = startIndex + numItems; | ||
| const pageItems = allSharedSecrets.slice(startIndex, endIndex); | ||
| const isDone = endIndex >= allSharedSecrets.length; | ||
| const continueCursor = isDone ? "" : endIndex.toString(); |
There was a problem hiding this comment.
(Refers to lines 619-686)
🚩 Pagination implementation fetches all data then slices - not true server-side pagination
The paginatedListByUser query at convex/sharedSecrets.ts:624-739 implements manual pagination by fetching ALL shared secrets (user-created + owner-projects), sorting them in memory, then slicing based on cursor. This means every pagination request loads the complete dataset into memory. For users with many shared secrets, this defeats the purpose of pagination. The comment at line 620-622 acknowledges this limitation. True Convex pagination with .paginate() cannot be used here because the data comes from multiple sources that need deduplication. This is a performance concern rather than a bug, but worth noting for future optimization.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ctor error handling and role limits
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/environment-variables/VariableEditRow.tsx (1)
71-86:⚠️ Potential issue | 🟡 MinorPaste handler may bypass maxLength constraint.
When pasting
KEY=VALUE, thehandleNamePastefunction callsonNameChange(key)directly without truncating or validating againstMAX_LENGTHS.VARIABLE_NAME. A pasted key longer than 50 characters will be set, only to be truncated on the next keystroke.Consider truncating the key before setting it.
🛡️ Proposed fix
+import { MAX_LENGTHS } from "@/lib/constants"; + // In handleNamePaste, after extracting key: - onNameChange(key); + onNameChange(key.slice(0, MAX_LENGTHS.VARIABLE_NAME));Apply the same fix in
handleValuePasteat Line 125.
🤖 Fix all issues with AI agents
In `@convex/environments.ts`:
- Around line 1-6: Reorder the import statements so they follow external
packages → internal modules → relative imports ordering: keep the external
import "v" from "convex/values" first, then internal imports like "mutation",
"query", and "QueryCtx" from "./_generated/server" and functions
"checkAndClearPlanEnforcementFlag", "getProjectOwnerLimits" from
"./lib/roleLimits" and "throwError", "validateLength" from "./lib/errors", and
finally the relative import of "FALLBACK_USER_DATA" and "MAX_LENGTHS" from
"../src/lib/constants"; update the imports in convex/environments.ts accordingly
to satisfy the linter.
In `@convex/lib/roleLimits.ts`:
- Around line 11-18: The current imports mix value and type imports and are out
of the linting order; change Tier and TierLimits to type-only imports and
reorder so value imports come first (alphabetically), then type imports
(alphabetically), and finally relative type imports. Specifically, import the
runtime symbols TIER_LIMITS, getTierLimits (as getTierLimitsShared), and
canPerformAction (as canPerformActionShared) as normal value imports
(alphabetically), then use import type for Tier and TierLimits, and keep import
type { Id } from "../_generated/dataModel" as the relative type import last.
In `@convex/projects.ts`:
- Around line 1-10: The import statements in convex/projects.ts are out of the
required grouping order; reorder them so external packages come first (e.g.,
import { v } from "convex/values"), then internal modules (e.g., import {
mutation, query } from "./_generated/server" and the ./lib/* imports like
checkAndClearPlanEnforcementFlag, getProjectOwnerLimits, getTierLimits,
throwError, validateLength), and finally relative imports (e.g., import {
MAX_LENGTHS } from "../src/lib/constants" and the type import for QueryCtx).
Ensure imports remain named the same and preserve existing symbols (v, mutation,
query, checkAndClearPlanEnforcementFlag, getProjectOwnerLimits, getTierLimits,
throwError, validateLength, MAX_LENGTHS, QueryCtx) while adjusting only their
order to match external → internal → relative grouping.
In `@convex/sharedSecrets.ts`:
- Around line 602-665: In paginatedListByUser, guard parsing of
args.paginationOpts.cursor so parseInt can't produce NaN or negative indices:
read args.paginationOpts.cursor into a local, parse with parseInt, check
Number.isFinite/Number.isInteger or isNaN and clamp to 0 (e.g., const startIndex
= Math.max(0, parsed || 0)), then compute endIndex = startIndex + numItems and
continueCursor as before; ensure startIndex is a safe integer before slicing
allSharedSecrets to avoid NaN/negative propagation into pageItems, isDone, and
continueCursor.
- Around line 1-8: Reorder the import statements in convex/sharedSecrets.ts to
satisfy the import/order rule: put external packages first (keep "convex/values"
and "convex/server" at the top), then internal project modules (move
"../src/lib/constants" next), and finally relative/local imports (place
"./_generated/server", "./lib/roleLimits", "./lib/errors" and the type imports
for QueryCtx and Id after that). Ensure related named imports (mutation, query)
and their type-only imports (QueryCtx, Id) remain associated with their source
modules when reordering.
In `@src/components/environment-variables/EnvironmentVariables.tsx`:
- Around line 17-31: Reorder the import statements in EnvironmentVariables.tsx
to satisfy lint grouping: place external package imports first (none here), then
internal absolute imports (e.g., imports starting with "@/": TIER_LIMITS,
EnvironmentVariablesProps/ParsedVariable types, variablesToExport, decrypt,
encrypt, formatRelativeTime, getErrorMessage), and finally relative imports (the
convex api import and component imports like api, ShareDialog, VariableRow,
VariableEditRow, BulkAddDialog, BulkEditDialog, useVariableActions, and Id).
Update the sequence so all "@/..." imports are grouped together above the
"../../../..." and "./..." imports while preserving existing identifiers
(TIER_LIMITS, api, ShareDialog, VariableRow, VariableEditRow, BulkAddDialog,
BulkEditDialog, useVariableActions, Id, EnvironmentVariablesProps,
ParsedVariable, variablesToExport, decrypt, encrypt, formatRelativeTime,
getErrorMessage).
🧹 Nitpick comments (9)
src/components/share-dialog.tsx (1)
5-5: Consolidate duplicate imports from@/lib/constants.ESLint flags that
@/lib/constantsis imported twice (Line 5 and Line 30) and the import order is incorrect. Merge these imports and place them after the type imports.♻️ Proposed fix
-import { MAX_LENGTHS } from "@/lib/constants"; - import type { ShareExpiryValue } from "@/lib/constants"; import type { Environment, Variable } from "@/lib/types"; +import { MAX_LENGTHS, SHARE_EXPIRY_OPTIONS } from "@/lib/constants"; import { Button } from "@/components/ui/button"; ... -import { SHARE_EXPIRY_OPTIONS } from "@/lib/constants";Also applies to: 30-31
convex/lib/errors.ts (2)
1-1: Useimport typefor type-only import.ESLint correctly flags that
Idis only used as a type (in theErrorContextinterface). This should use a type-only import.♻️ Proposed fix
-import { Id } from "convex/_generated/dataModel"; +import type { Id } from "convex/_generated/dataModel";
55-58: Consider adding context parameter tovalidateLength.For consistency with
throwError, consider allowing an optionalcontextparameter so validation errors can include relevant identifiers in server logs.♻️ Optional enhancement
-export function validateLength(val: string | undefined, max: number, fieldName: string) { +export function validateLength( + val: string | undefined, + max: number, + fieldName: string, + context?: ErrorContext +) { if (val && val.length > max) { - throwError(`${fieldName} is too long (max ${max} characters)`, "BAD_REQUEST", 400); + throwError(`${fieldName} is too long (max ${max} characters)`, "BAD_REQUEST", 400, context); } }src/lib/types.ts (1)
1-2: Fix import to useimport typeand correct import order.The
Tierimport is only used as a type, so it should useimport type. Additionally, per ESLint, the import order needs adjustment.♻️ Proposed fix
-import type { Id } from "../../convex/_generated/dataModel"; -import { Tier } from "./role-limits"; +import type { Tier } from "./role-limits"; +import type { Id } from "../../convex/_generated/dataModel";src/components/app-sidebar.tsx (1)
6-7: Fix import order: type import should come before value import.Per ESLint, the type import should occur before the value import from the same module.
♻️ Proposed fix
-import { TIER_LIMITS } from "@/lib/role-limits"; -import type { Tier } from "@/lib/role-limits"; +import type { Tier } from "@/lib/role-limits"; +import { TIER_LIMITS } from "@/lib/role-limits";src/routes/d/project/$projectId.tsx (1)
17-17: Fix import order:@/lib/constantsshould come after type imports.Per ESLint, non-type imports should be ordered after type imports.
♻️ Proposed fix
import { api } from "../../../../convex/_generated/api"; -import { MAX_LENGTHS } from "@/lib/constants"; import type { Id } from "../../../../convex/_generated/dataModel"; import type { Environment } from "@/lib/types"; +import { MAX_LENGTHS } from "@/lib/constants"; import { Button } from "@/components/ui/button";convex/users.ts (1)
1-6: Fix import order:../src/lib/constantsshould come before./_generated/server.Per ESLint, the import order needs adjustment.
♻️ Proposed fix
import { v } from "convex/values"; +import { MAX_LENGTHS } from "../src/lib/constants"; import { mutation, query } from "./_generated/server"; import { TIER_LIMITS } from "./lib/roleLimits"; import { throwError, validateLength } from "./lib/errors"; -import { MAX_LENGTHS } from "../src/lib/constants"; import type { Tier } from "./lib/roleLimits";convex/variables.ts (1)
1-7: Fix import order:../src/lib/constantsshould come before./_generated/server.Per ESLint, the import order needs adjustment.
♻️ Proposed fix
import { v } from "convex/values"; +import { MAX_LENGTHS } from "../src/lib/constants"; import { mutation, query } from "./_generated/server"; import { getProjectOwnerLimits } from "./lib/roleLimits"; import { throwError, validateLength } from "./lib/errors"; -import { MAX_LENGTHS } from "../src/lib/constants"; import type { QueryCtx } from "./_generated/server";src/lib/role-limits.ts (1)
8-17: Define Tier types insrc/lib/types.tsand re-export here.
These are shared across frontend + backend, so centralizing them in the shared types module keeps ownership clear and avoids duplicate definitions.♻️ Suggested refactor (this file)
-export type Tier = "free" | "pro" | "pro_plus" | "super_admin"; - -export interface TierLimits { - maxProjects: number; - maxEnvironmentsPerProject: number; - maxMembersPerProject: number; - maxSharedSecretsPerProject: number; - maxVariablesPerEnvironment: number; - canCreateIndefiniteShares: boolean; -} +import type { Tier, TierLimits } from "@/lib/types";Also add the
Tier/TierLimitsdeclarations tosrc/lib/types.tsand re-export as needed.
As per coding guidelines: Define shared types insrc/lib/types.ts.
There was a problem hiding this comment.
🟡 Shared dashboard empty state row uses wrong colSpan after adding selection column
/d/shared adds a new checkbox selection column, increasing the number of table columns, but the empty-state row still uses colSpan={8}.
Actual: when there are no rows, the “No shared secrets found” cell will not span the full width, causing misaligned layout.
Expected: empty-state cell should span all columns.
Click to expand
The header now includes the checkbox column (src/routes/d/shared.tsx:349-367), but the empty row remains:
<TableCell colSpan={8} className="h-32 text-center text-muted-foreground">
No shared secrets found
</TableCell>src/routes/d/shared.tsx:373-375
(Refers to lines 373-375)
Recommendation: Update colSpan to match the actual column count (likely 9 after adding the selection column).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
convex/users.ts (1)
24-35:⚠️ Potential issue | 🟡 MinorValidate the derived display name, not just
args.name.When
args.nameis undefined, the email local‑part can exceed MAX_LENGTHS.USER_NAME and bypass validation.Suggested fix
- validateLength(args.name, MAX_LENGTHS.USER_NAME, "Name"); validateLength(args.email, MAX_LENGTHS.USER_EMAIL, "Email"); - // Check if the user already exists + const name = (args.name || args.email.split("@")[0] || "Unknown User").trim(); + validateLength(name, MAX_LENGTHS.USER_NAME, "Name"); + + // Check if the user already exists @@ - const name = (args.name || args.email.split("@")[0] || "Unknown User").trim();src/routes/d/project/$projectId.tsx (1)
1-53:⚠️ Potential issue | 🟡 MinorReorder imports to keep internal
@/modules before relative imports.🧹 Suggested reorder
-import { api } from "../../../../convex/_generated/api"; -import type { Id } from "../../../../convex/_generated/dataModel"; - import type { Environment } from "@/lib/types"; import { MAX_LENGTHS } from "@/lib/constants"; import { Button } from "@/components/ui/button"; @@ import { keyStore } from "@/lib/key-store"; import { getErrorMessage } from "@/lib/errors"; + +import { api } from "../../../../convex/_generated/api"; +import type { Id } from "../../../../convex/_generated/dataModel";As per coding guidelines: Group imports in order: external packages → internal modules → relative imports.
🤖 Fix all issues with AI agents
In `@convex/projects.ts`:
- Around line 74-92: This change adds tier-based project limits (getTierLimits,
existingProjects check, throwError) and server-side validation (validateLength,
MAX_LENGTHS), so update the package.json version to a new semver release
appropriate for these non-breaking feature/behavior changes (e.g., bump minor
version), commit the updated version, and ensure package-lock or yarn.lock is
updated accordingly before merging.
In `@src/components/project-settings.tsx`:
- Around line 14-18: Move the MAX_LENGTHS import into the internal/module group
so that import ordering is: external packages (useNavigate), internal aliases
(MAX_LENGTHS, getErrorMessage), then relative Convex imports (api, Id);
specifically place the "@/lib/constants" import alongside "@/lib/errors"
(getErrorMessage) and ensure "../../convex/_generated/api" (api) and
"../../convex/_generated/dataModel" (Id) follow after those.
🧹 Nitpick comments (5)
convex/lib/errors.ts (2)
35-46: Log the message even when no context is provided.Right now the log prints an empty string when context is absent, which drops the message.
Suggested tweak
- console.warn(`[${type}:${code}]`, context ? JSON.stringify(payload, null, 2) : ""); + console.warn(`[${type}:${code}]`, JSON.stringify(payload, null, 2));
55-58: Add an explicit return type forvalidateLength.Suggested change
-export function validateLength(val: string | undefined, max: number, fieldName: string) { +export function validateLength( + val: string | undefined, + max: number, + fieldName: string +): void {As per coding guidelines: Prefer explicit return types for exported functions.
convex/sharedSecrets.ts (2)
169-215: Avoid repeating auth/user lookups per ID in bulk operations.
checkSecretManagementAccessperforms identity + user queries each call; in bulk loops this repeats N times. Consider splitting into a “getAuthenticatedUser” step once and a per‑secret access check that reuses the user object to reduce DB hits.Also applies to: 464-492, 566-595
305-309: Use theby_userIdindex for membership lookup.The
by_userIdindex exists in the schema and is the appropriate index for this query. Using it avoids a collection scan and matches the index usage pattern elsewhere in this function.Suggested change
- const allMemberships = await ctx.db - .query("projectMembers") - .filter((q) => q.eq(q.field("userId"), user._id)) - .collect(); + const allMemberships = await ctx.db + .query("projectMembers") + .withIndex("by_userId", (q) => q.eq("userId", user._id)) + .collect();src/components/environment-variables/EnvironmentVariables.tsx (1)
63-68: Add an explicit return type to the exported component.✏️ Suggested update
-export function EnvironmentVariables({ +export function EnvironmentVariables({ environment, derivedKey, userRole, ownerTier, -}: EnvironmentVariablesProps) { +}: EnvironmentVariablesProps): JSX.Element {As per coding guidelines: Prefer explicit return types for exported functions.
| import { useNavigate } from "@tanstack/react-router"; | ||
| import { api } from "../../convex/_generated/api"; | ||
| import type { Id } from "../../convex/_generated/dataModel"; | ||
| import { MAX_LENGTHS } from "@/lib/constants"; | ||
| import { getErrorMessage } from "@/lib/errors"; |
There was a problem hiding this comment.
Reorder imports to keep internal aliases before relative Convex imports.
The new MAX_LENGTHS alias should be grouped with the other @/ imports, and the relative Convex imports should follow.
Suggested reorder
import { useNavigate } from "@tanstack/react-router";
-import { api } from "../../convex/_generated/api";
-import type { Id } from "../../convex/_generated/dataModel";
import { MAX_LENGTHS } from "@/lib/constants";
import { getErrorMessage } from "@/lib/errors";
@@
import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs";
import { hash as cryptoHash } from "@/lib/crypto";
+import { api } from "../../convex/_generated/api";
+import type { Id } from "../../convex/_generated/dataModel";As per coding guidelines: Group imports in order: external packages → internal modules → relative imports.
📝 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.
| import { useNavigate } from "@tanstack/react-router"; | |
| import { api } from "../../convex/_generated/api"; | |
| import type { Id } from "../../convex/_generated/dataModel"; | |
| import { MAX_LENGTHS } from "@/lib/constants"; | |
| import { getErrorMessage } from "@/lib/errors"; | |
| import { useNavigate } from "@tanstack/react-router"; | |
| import { MAX_LENGTHS } from "@/lib/constants"; | |
| import { getErrorMessage } from "@/lib/errors"; | |
| import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; | |
| import { hash as cryptoHash } from "@/lib/crypto"; | |
| import { api } from "../../convex/_generated/api"; | |
| import type { Id } from "../../convex/_generated/dataModel"; |
🤖 Prompt for AI Agents
In `@src/components/project-settings.tsx` around lines 14 - 18, Move the
MAX_LENGTHS import into the internal/module group so that import ordering is:
external packages (useNavigate), internal aliases (MAX_LENGTHS,
getErrorMessage), then relative Convex imports (api, Id); specifically place the
"@/lib/constants" import alongside "@/lib/errors" (getErrorMessage) and ensure
"../../convex/_generated/api" (api) and "../../convex/_generated/dataModel" (Id)
follow after those.
…d secrets display
1 participant
Description
Summary by CodeRabbit
New Features
Improvements
Removals
Documentation
Checklist
Additional Information