-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add optional per-user drawing limit with bypass list via environment variables #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
…ve limit Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
|
@copilot can you also add an environment variable that lets me define users that bypass this max drawing limit? I should be able to put multiple users. I should put in their account email |
Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
Added Usage: VITE_UNLIMITED_USERS=admin@example.com,manager@example.com,premium@example.comThe email comparison is case-insensitive. Implemented in commits b599b3b and c2df17e with performance optimizations (caching and efficient queries). |
|
@copilot can you resolve all merge conflicts |
…ature Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
Resolved all merge conflicts in commit 415d627. The conflicts were in:
Both features now work together: the drawing limit is checked first, then if a folder isn't specified, a default folder is automatically created. Build passed successfully. |
|
@copilot resolve merge conflict |
Resolved merge conflict in commit a95a97e. The conflict was in Build passed successfully and all changes are now pushed. |
|
@copilot even if there is less than 5 drawings i am still getting the error that I already have 5 drawings so it wont let me create more drawings. investigate that please and come up with a plan to fix it before actually fixing it |
Investigation & Fix PlanProblem: Users getting "already have 5 drawings" error even when they have less than 5 drawings. Root Cause AnalysisAfter investigating the code, I identified the most likely issue: The count query returns
Proposed Fix
Changes to make:// Line 99: Change from
if (count !== null && count >= limit)
// To
if (count !== null && count > limit)This ensures users can create UP TO the limit, not up to limit-1. Ready to implement the fix?
Fixed in commit 059417cRoot Cause: The count query used The Fix: .eq("is_deleted", false) // Only counts explicit falseTo: .not('is_deleted', 'eq', true) // Counts everything except explicit true (includes NULL)This ensures all non-deleted drawings are counted, whether Additional improvements:
The console log will help debug any remaining issues. Check your browser console when creating drawings to see the actual count values. |
Co-authored-by: notkainoa <123281048+notkainoa@users.noreply.github.com>
|
@copilot review all changes for any bugs or errors or anything |
Code Review Complete ✅Reviewed all 8 commits and changes across 23 files. Overall: Code is production-ready with only minor improvements suggested. Build Status✅ Build passes successfully Code Quality: EXCELLENT
Issues FoundMinor Issues (Non-blocking):
Security: ACCEPTABLE✅ No SQL injection risks Note: For hard limits, consider adding Supabase RLS policies server-side. What Works Great✅ NULL value handling in count query (bug fix) Verdict: Ship it! 🚀 Minor issues are acceptable for the use case. |
| const { count, error: countError } = await supabase | ||
| .from(DB_NAME) | ||
| .select('page_id', { count: 'exact', head: true }) | ||
| .eq("user_id", profile.user?.id) | ||
| .not('is_deleted', 'eq', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The code incorrectly accesses profile.user?.id to get the user ID. The profile variable is the user object itself, so profile.user is undefined, causing the check to fail.
Severity: HIGH
Suggested Fix
Correct the property access for the user ID. Instead of using profile.user?.id, use profile?.id to correctly reference the ID on the user object. This will ensure the database query to count drawings uses the correct user ID.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/db/draw.ts#L90-L94
Potential issue: The `createNewPage` function fetches the user with
`supabase.auth.getUser()` and assigns the `data` object to a variable named `profile`.
The code then attempts to access the user's ID via `profile.user?.id`. However, the
`profile` variable is the user object itself, not an object containing a `user`
property. This means `profile.user` is always `undefined`, and the database query to
count existing drawings is performed with an `undefined` user ID. This causes the query
to fail, and the error is caught, preventing any user from creating new drawings as the
limit check can never be successfully completed.
Did we get this right? 👍 / 👎 to inform future reviews.
Implementation Plan
VITE_MAX_DRAWINGS_PER_USERcreateNewPagefunction to check limit before creationVITE_UNLIMITED_USERSbypass list featureSummary
This PR implements an optional drawing limit feature controlled by the
VITE_MAX_DRAWINGS_PER_USERenvironment variable, with a bypass list viaVITE_UNLIMITED_USERS.Key Features:
Recent Fix:
Changed count query filter from
.eq("is_deleted", false)to.not('is_deleted', 'eq', true)This ensures the count includes:
is_deleted = false(explicitly not deleted)is_deleted = NULL(default state for new drawings)is_deleted = true(explicitly deleted)Also added:
Changes:
createNewPageto check bypass list and enforce limit before creating drawingsis_deletedvaluespage_idinstead of all columns.env.examplewithVITE_UNLIMITED_USERSdocumentationUsage:
Users in the bypass list can create unlimited drawings even when
VITE_MAX_DRAWINGS_PER_USERis set. The drawing limit check happens before the default folder creation, ensuring limits are enforced consistently.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
TL;DR
Implemented an optional per-user drawing limit configurable via
VITE_MAX_DRAWINGS_PER_USER, with a bypass list for specific users set byVITE_UNLIMITED_USERS. The feature includes clear error messages, accurate non-deleted drawing counts, and optimized performance.What changed?
createNewPageto check bypass list and enforce the drawing limit before creation.NULLvalues foris_deleted, ensuring only non-deleted drawings are counted.VITE_UNLIMITED_USERSbypass list to improve performance.page_idfor efficiency..env.exampleandREADMEwith documentation for the newVITE_MAX_DRAWINGS_PER_USERandVITE_UNLIMITED_USERSenvironment variables.mainbranch.Description generated by Mesa. Update settings