FEAT: add advanced campaign filters and UI for viewing campaigns #145
Conversation
WalkthroughAdds a Campaign Management feature: extensive SQL schema for brands/campaigns/creators/deals and related enums, a FastAPI campaigns router with CRUD and ownership checks, frontend pages/components/types for listing and creating campaigns, an API client, and supporting documentation and guides. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Brand User
participant Frontend as Frontend (React)
participant Auth as Supabase Auth
participant Backend as FastAPI (campaigns)
participant DB as Database
Note over Frontend,Auth: Create Campaign flow
User->>Frontend: Open Create Campaign page
Frontend->>Auth: Verify session & get user_id
Auth-->>Frontend: user_id
User->>Frontend: Submit campaign form
Frontend->>Backend: POST /campaigns (user_id + payload)
Backend->>Backend: get_brand_id_from_user(user_id)
Backend->>DB: SELECT brands WHERE user_id = ?
DB-->>Backend: brand_id
Backend->>Backend: generate slug, set published_at if active
Backend->>DB: INSERT INTO public.campaigns (...)
DB-->>Backend: created record
Backend-->>Frontend: 201 CampaignResponse
Frontend-->>User: Navigate to campaigns list
sequenceDiagram
participant User as Brand User
participant Frontend as Frontend (React)
participant Backend as FastAPI (campaigns)
participant DB as Database
Note over Frontend,Backend: List & filter flow
User->>Frontend: Open Campaigns list
Frontend->>Backend: GET /campaigns?user_id=...&filters
Backend->>Backend: resolve brand_id from user_id
Backend->>DB: SELECT campaigns WHERE brand_id AND filters
DB-->>Backend: campaigns[]
Backend-->>Frontend: List[CampaignResponse]
Frontend->>Frontend: format & render cards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–70 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
backend/SQL(2 hunks)backend/app/api/routes/campaigns.py(1 hunks)backend/app/api/routes/health.py(1 hunks)backend/app/main.py(2 hunks)frontend/app/brand/campaigns/create/page.tsx(1 hunks)frontend/app/brand/campaigns/page.tsx(1 hunks)frontend/app/brand/createcampaign/page.tsx(1 hunks)frontend/lib/campaignApi.ts(1 hunks)frontend/types/campaign.ts(1 hunks)guides/summaries/CAMPAIGN_MANAGEMENT_IMPLEMENTATION.md(1 hunks)guides/summaries/CAMPAIGN_QUICK_REFERENCE.md(1 hunks)guides/summaries/CAMPAIGN_VISUAL_FLOW.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
guides/summaries/CAMPAIGN_MANAGEMENT_IMPLEMENTATION.md
49-49: Bare URL used
(MD034, no-bare-urls)
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
guides/summaries/CAMPAIGN_VISUAL_FLOW.md
270-270: Bare URL used
(MD034, no-bare-urls)
271-271: Bare URL used
(MD034, no-bare-urls)
272-272: Bare URL used
(MD034, no-bare-urls)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
338-338: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
343-343: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
349-349: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.3)
backend/app/api/routes/campaigns.py
84-84: Abstract raise to an inner function
(TRY301)
87-87: Do not catch blind exception: Exception
(BLE001)
89-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Use explicit conversion flag
Replace with conversion flag
(RUF010)
141-141: Abstract raise to an inner function
(TRY301)
145-145: Do not catch blind exception: Exception
(BLE001)
146-146: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
146-146: Use explicit conversion flag
Replace with conversion flag
(RUF010)
158-158: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
214-214: Consider moving this statement to an else block
(TRY300)
216-216: Do not catch blind exception: Exception
(BLE001)
219-222: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
221-221: Use explicit conversion flag
Replace with conversion flag
(RUF010)
246-246: Abstract raise to an inner function
(TRY301)
248-248: Consider moving this statement to an else block
(TRY300)
250-250: Do not catch blind exception: Exception
(BLE001)
252-252: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
253-253: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
253-253: Use explicit conversion flag
Replace with conversion flag
(RUF010)
279-279: Abstract raise to an inner function
(TRY301)
285-285: Abstract raise to an inner function
(TRY301)
300-300: Abstract raise to an inner function
(TRY301)
306-306: Do not catch blind exception: Exception
(BLE001)
308-308: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
309-309: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
309-309: Use explicit conversion flag
Replace with conversion flag
(RUF010)
333-333: Abstract raise to an inner function
(TRY301)
338-338: Consider moving this statement to an else block
(TRY300)
342-342: Do not catch blind exception: Exception
(BLE001)
344-344: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
345-345: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
345-345: Use explicit conversion flag
Replace with conversion flag
(RUF010)
frontend/lib/campaignApi.ts
Outdated
| budget_min: formData.budget_min | ||
| ? parseFloat(formData.budget_min) | ||
| : undefined, | ||
| budget_max: formData.budget_max | ||
| ? parseFloat(formData.budget_max) | ||
| : undefined, | ||
| preferred_creator_niches: formData.preferred_creator_niches, | ||
| preferred_creator_followers_range: | ||
| formData.preferred_creator_followers_range || undefined, | ||
| starts_at: formData.starts_at || undefined, | ||
| ends_at: formData.ends_at || undefined, | ||
| }; |
There was a problem hiding this comment.
Fix budget parsing so zero and cleared values persist
formData.budget_min / formData.budget_max are strings, so the truthy checks skip "0" and any deliberate clear (""), meaning we silently drop valid inputs and make it impossible to set a zero minimum or clear an existing amount. Persisted filters and edits will therefore never reflect those values. Please normalise by checking explicitly for undefined / empty string and coercing to a number (or null when clearing).
- budget_min: formData.budget_min
- ? parseFloat(formData.budget_min)
- : undefined,
- budget_max: formData.budget_max
- ? parseFloat(formData.budget_max)
- : undefined,
+ budget_min:
+ formData.budget_min !== undefined && formData.budget_min !== ""
+ ? Number(formData.budget_min)
+ : undefined,
+ budget_max:
+ formData.budget_max !== undefined && formData.budget_max !== ""
+ ? Number(formData.budget_max)
+ : undefined,
@@
- if (formData.budget_min) payload.budget_min = parseFloat(formData.budget_min);
- if (formData.budget_max) payload.budget_max = parseFloat(formData.budget_max);
+ if (formData.budget_min !== undefined) {
+ payload.budget_min =
+ formData.budget_min === "" ? null : Number(formData.budget_min);
+ }
+ if (formData.budget_max !== undefined) {
+ payload.budget_max =
+ formData.budget_max === "" ? null : Number(formData.budget_max);
+ }Also applies to: 166-175
🤖 Prompt for AI Agents
In frontend/lib/campaignApi.ts around lines 113-124 (and likewise update lines
166-175), the budget parsing uses truthy checks which drop valid "0" and prevent
clearing with "" — change the logic to explicitly test for undefined or empty
string: if formData.budget_min (or budget_max) is not undefined and not an empty
string, coerce it to a number with parseFloat; otherwise set the field to null
(or undefined per your API contract) so clearing is persisted. Ensure you apply
the same explicit check/coercion for both min and max in both code locations.
…scription, and description
…-safe and match backend
…aign creation and filtering
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/SQL (1)
3-9: Remove duplicateprofilestable definition.The
profilestable is defined twice in this script: once at lines 3–9 and again at lines 323–331. The second definition includes an additionalonboarding_completedcolumn but otherwise duplicates the first. When executed, the secondCREATE TABLEwill fail because the table already exists.Remove one definition. If you need to add the
onboarding_completedcolumn to an existing table, use:ALTER TABLE public.profiles ADD COLUMN IF NOT EXISTS onboarding_completed boolean DEFAULT false;Also applies to: 323-331
🧹 Nitpick comments (4)
frontend/app/brand/campaigns/create/page.tsx (2)
206-213: Disable form inputs during submission.The submit buttons are disabled during loading (lines 657, 666), but all form inputs remain enabled. Users can modify fields while the submission is in progress, which could cause confusion or unexpected behavior if the form is resubmitted.
Add the
disabled={loading}prop to all form inputs and interactive elements:<input type="text" value={formData.title} onChange={(e) => updateField("title", e.target.value)} placeholder="e.g., Summer Product Launch 2024" + disabled={loading} className="w-full rounded-lg border border-gray-300 px-4 py-3 focus:border-blue-500 focus:ring-2 focus:ring-blue-200 focus:outline-none" required />Apply the same change to all other inputs, textareas, selects, and interactive buttons throughout the form.
Also applies to: 220-228, 235-242, 256-262, 267-273, 287-295, 300-308
119-124: Add validation for negative budget values.The validation checks that
budget_minis less than or equal tobudget_max, but it doesn't verify that budget values are positive. Users could enter negative numbers, which would pass validation but represent invalid data.Add a check for non-negative budgets:
if (formData.budget_min && formData.budget_max) { + const minBudget = parseFloat(formData.budget_min); + const maxBudget = parseFloat(formData.budget_max); + if (minBudget < 0 || maxBudget < 0) { + setError("Budget values must be positive"); + return false; + } - if (parseFloat(formData.budget_min) > parseFloat(formData.budget_max)) { + if (minBudget > maxBudget) { setError("Minimum budget cannot be greater than maximum budget"); return false; } }backend/app/api/routes/campaigns.py (2)
133-135: Documentpublished_atbehavior for status transitions.The code sets
published_atwhen a campaign's status becomes "active" but preserves the original timestamp if the campaign is re-activated after being paused/drafted. This meanspublished_atrepresents the first publication date, not the most recent activation.Consider adding a docstring or comment to clarify this behavior:
# If status is active, set published_at + # Note: published_at represents the first time the campaign was published + # and is not updated on subsequent activations if campaign.status == "active": campaign_data["published_at"] = datetime.now().isoformat()Additionally, if the business requires tracking the most recent activation, add a separate
last_activated_atfield.Also applies to: 299-303
14-30: Add Pydantic validators for cross-field validation.The Pydantic models validate individual field types but don't enforce relationships between fields (e.g.,
budget_min ≤ budget_max,starts_at ≤ ends_at). While the frontend performs these checks, backend validation is essential to prevent invalid data when the API is called directly or from other clients.Add field validators to both
CampaignCreateandCampaignUpdate:from pydantic import field_validator, model_validator class CampaignCreate(BaseModel): # ... existing fields ... @field_validator('budget_min', 'budget_max') @classmethod def validate_budget_positive(cls, v): if v is not None and v < 0: raise ValueError('Budget must be non-negative') return v @model_validator(mode='after') def validate_budget_range(self): if self.budget_min is not None and self.budget_max is not None: if self.budget_min > self.budget_max: raise ValueError('budget_min must be less than or equal to budget_max') return self @model_validator(mode='after') def validate_date_range(self): if self.starts_at is not None and self.ends_at is not None: if self.starts_at > self.ends_at: raise ValueError('starts_at must be before or equal to ends_at') return selfApply similar validators to
CampaignUpdate.Also applies to: 33-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/SQL(2 hunks)backend/app/api/routes/campaigns.py(1 hunks)frontend/app/brand/campaigns/create/page.tsx(1 hunks)frontend/app/brand/campaigns/page.tsx(1 hunks)frontend/lib/campaignApi.ts(1 hunks)frontend/types/campaign.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/brand/campaigns/page.tsx
- frontend/lib/campaignApi.ts
🧰 Additional context used
🪛 Ruff (0.14.3)
backend/app/api/routes/campaigns.py
84-84: Abstract raise to an inner function
(TRY301)
87-87: Do not catch blind exception: Exception
(BLE001)
89-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Use explicit conversion flag
Replace with conversion flag
(RUF010)
141-141: Abstract raise to an inner function
(TRY301)
145-145: Do not catch blind exception: Exception
(BLE001)
146-146: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
146-146: Use explicit conversion flag
Replace with conversion flag
(RUF010)
158-158: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
214-214: Consider moving this statement to an else block
(TRY300)
216-216: Do not catch blind exception: Exception
(BLE001)
219-222: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
221-221: Use explicit conversion flag
Replace with conversion flag
(RUF010)
246-246: Abstract raise to an inner function
(TRY301)
248-248: Consider moving this statement to an else block
(TRY300)
250-250: Do not catch blind exception: Exception
(BLE001)
252-252: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
253-253: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
253-253: Use explicit conversion flag
Replace with conversion flag
(RUF010)
279-279: Abstract raise to an inner function
(TRY301)
286-286: Abstract raise to an inner function
(TRY301)
309-309: Abstract raise to an inner function
(TRY301)
315-315: Do not catch blind exception: Exception
(BLE001)
317-317: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
318-318: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
318-318: Use explicit conversion flag
Replace with conversion flag
(RUF010)
342-342: Abstract raise to an inner function
(TRY301)
347-347: Consider moving this statement to an else block
(TRY300)
351-351: Do not catch blind exception: Exception
(BLE001)
353-353: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
354-354: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
354-354: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (3)
frontend/types/campaign.ts (1)
1-187: LGTM! Well-structured type definitions.The type definitions are well-organized with clear separation between:
- Form state types (
CampaignFormDatawith string values for inputs)- API payload types (
CampaignPayloadwith proper numeric types)- Response types (
Campaignmatching the backend schema)- Filter types (
CampaignFiltersfor query parameters)The use of
as constfor option arrays provides proper type narrowing, and optional fields are correctly marked. The types align well with the backend schema and API expectations.backend/app/api/routes/campaigns.py (2)
190-192: Verify PostgREST query syntax and consider SQL injection protection.The search filter directly interpolates the user-provided
searchparameter into the PostgREST query string. While PostgREST should handle escaping, verify that:
- The
.or_()syntax correctly accepts this format- Special PostgREST operators in the search term (e.g.,
eq.,ilike.) won't be interpreted as query operators- The
descriptionfield being nullable doesn't cause the filter to exclude campaigns with null descriptionsTest with these search values to ensure they're handled safely:
"eq.malicious"(PostgREST operator injection attempt)"title,description"(comma could be misinterpreted)"%' OR '1'='1"(SQL-style injection attempt, should be harmless but verify)If the current implementation is vulnerable, use Supabase's parameterized filtering or escape special characters before interpolation.
197-201: Verify budget filter logic matches business requirements.The budget filtering uses:
query.gte("budget_min", budget_min)— finds campaigns where campaign's minimum budget ≥ filter's budget_minquery.lte("budget_max", budget_max)— finds campaigns where campaign's maximum budget ≤ filter's budget_maxThis logic finds campaigns with budget ranges that fall within or overlap the filter range. Verify this matches the intended behavior:
Current behavior example:
- User filters with
budget_min=50000, budget_max=200000- Matches campaign with
budget_min=60000, budget_max=150000✓- Excludes campaign with
budget_min=40000, budget_max=180000(min too low) ✗- Excludes campaign with
budget_min=60000, budget_max=250000(max too high) ✗Alternative interpretation:
If the intent is "show campaigns I can afford within my budget range," the logic should be:
query.gte("budget_min", budget_max)— campaign min doesn't exceed my maxquery.lte("budget_max", budget_min)— campaign max isn't below my minConfirm with the product team which filtering behavior is expected and adjust the operators accordingly.
backend/SQL
Outdated
| -- WARNING: This schema is for context only and is not meant to be run. | ||
| -- Table order and constraints may not be valid for execution. | ||
| -- i got this sceme from clicking the save as sql button in supabase |
There was a problem hiding this comment.
Clarify the purpose and location of this schema file.
The warning states this schema is "for context only and is not meant to be run," yet it contains executable DDL statements and is committed to the repository. This creates confusion about whether this is a migration script, documentation, or a dump.
Consider one of the following:
- If this is reference documentation, move it to a
docs/directory and convert it to markdown with explanatory notes. - If this is a migration, remove the warning and ensure it's properly ordered and executable.
- If this is a Supabase export, consider using a proper migration tool instead of committing raw dumps.
🤖 Prompt for AI Agents
In backend/SQL around lines 10-12, the top-of-file warning and committed
executable DDL create confusion about this file's purpose; decide and document
one: if it's reference docs, move the SQL to docs/ as a Markdown file with
explanatory notes and non-executable fenced SQL blocks; if it's an actual
migration, remove the warning, reorder and validate DDL so it runs cleanly and
add it to the migrations pipeline with proper filename/metadata; if it's a
Supabase export, replace it with a proper migration generated by your migration
tool or store the raw dump in a dedicated exports/ or backups/ folder and
reference it in README.
| CREATE TABLE public.campaign_applications ( | ||
| id uuid NOT NULL DEFAULT uuid_generate_v4(), | ||
| campaign_id uuid NOT NULL, | ||
| creator_id uuid NOT NULL, | ||
| profile_snapshot jsonb DEFAULT '{}'::jsonb, | ||
| message text, | ||
| proposed_amount numeric, | ||
| attachments jsonb DEFAULT '[]'::jsonb, | ||
| status USER-DEFINED DEFAULT 'applied'::application_status, | ||
| created_at timestamp with time zone DEFAULT now(), | ||
| updated_at timestamp with time zone DEFAULT now(), | ||
| CONSTRAINT campaign_applications_pkey PRIMARY KEY (id), | ||
| CONSTRAINT campaign_applications_campaign_id_fkey FOREIGN KEY (campaign_id) REFERENCES public.campaigns(id), | ||
| CONSTRAINT campaign_applications_creator_id_fkey FOREIGN KEY (creator_id) REFERENCES public.creators(id) | ||
| ); |
There was a problem hiding this comment.
Reorder table definitions to satisfy foreign key dependencies.
Several tables reference foreign keys to tables that are defined later in the script:
campaign_applications(line 98) referencescampaigns(defined at line 187) andcreators(defined at line 211)campaign_assets(line 113) referencescampaignsanddeals(defined at line 292)campaign_deliverables(line 127) referencescampaignscampaign_invites(line 139) referencescampaigns,brands, andcreatorscampaign_payments(line 155) referencesdealscampaign_performance(line 169) referencescampaignsanddeals
This ordering will cause foreign key constraint errors during execution.
Reorder the tables so that referenced tables are created before tables that reference them:
profilesbrands(referencesprofiles)creators(referencesprofiles)campaigns(referencesbrands)campaign_applications(referencescampaigns,creators)campaign_deliverables(referencescampaigns)campaign_invites(referencescampaigns,brands,creators)deals(referencescampaigns,campaign_applications,brands,creators)campaign_assets(referencescampaigns,deals)campaign_payments(referencesdeals)campaign_performance(referencescampaigns,deals)match_scores(referencescreators,brands)
Also applies to: 113-126, 127-138, 139-154, 155-168, 169-186
🤖 Prompt for AI Agents
In backend/SQL around lines 98-112 (and similarly for the blocks at 113-126,
127-138, 139-154, 155-168, 169-186), the CREATE TABLE statements are ordered so
tables with foreign keys reference tables that are defined later, causing FK
constraint errors; reorder the script so referenced tables are created before
their dependents following the sequence: profiles, brands, creators, campaigns,
campaign_applications, campaign_deliverables, campaign_invites, deals,
campaign_assets, campaign_payments, campaign_performance, match_scores; move
each CREATE TABLE block to that sequence and ensure any FK constraints reference
existing tables (or convert to ALTER TABLE ... ADD CONSTRAINT after target table
creation if reordering is infeasible).
There was a problem hiding this comment.
The SQL schema in SQL is not meant to be executed directly, and the file now only contains a pointer to the new documentation in schema-reference.md The actual schema reference is now in Markdown format for documentation purposes, not for execution.
There was a problem hiding this comment.
@Saahi30, thank you for the clarification! I understand now that the SQL file is purely for documentation and reference purposes, not for execution. The warning comment in the file makes this clear.
Since the schema is not meant to be run directly and serves as a reference pointing to the proper documentation in schema-reference.md, the table ordering concern I raised is not applicable. I appreciate you taking the time to explain the file's purpose.
✏️ Learnings added
Learnt from: Saahi30
Repo: AOSSIE-Org/InPactAI PR: 145
File: backend/SQL:98-112
Timestamp: 2025-11-08T12:24:11.575Z
Learning: When a SQL file in the backend/SQL directory contains a warning comment indicating it is "for context only" or "not meant to be run," do not flag table ordering or foreign key dependency issues, as the file is for documentation purposes only, not execution.
Learnt from: Saahi30
Repo: AOSSIE-Org/InPactAI PR: 145
File: backend/SQL:98-112
Timestamp: 2025-11-08T12:24:11.575Z
Learning: In the InPactAI repository, the backend/SQL file is a documentation reference that points to schema-reference.md and is not meant to be executed directly. Table ordering and FK constraint issues should not be flagged for this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/SQL (1)
18-19: Clarify the relationship between this file and schema-reference.md.The deprecation notice states this file is "no longer maintained here" and points to
docs/database/schema-reference.md, yet the full schema (320+ lines) remains below. This creates confusion about the source of truth.Consider one of the following:
- If
schema-reference.mdis the canonical reference, replace this file's content with only the ENUM definitions (lines 1-15) and the pointer comment, removing the table definitions to prevent drift.- If this file remains authoritative, remove the deprecation notice and clarify that schema-reference.md is generated from or mirrors this file.
- If both serve different purposes, document the relationship explicitly (e.g., "This file contains executable DDL; schema-reference.md contains annotated documentation").
Based on learnings
🧹 Nitpick comments (7)
backend/SQL (1)
113-113: Consider replacing USER-DEFINED notation with explicit type names for clarity.The
USER-DEFINEDnotation on lines 113, 153, 168, and 308 is PostgreSQL's internal representation frompg_dumpoutput. For documentation purposes, explicitly naming the ENUM types would be clearer:- status USER-DEFINED DEFAULT 'applied'::application_status, + status application_status DEFAULT 'applied'::application_status,Apply similar changes to lines 153, 168, and 308 for
invite_status,payment_status, anddeal_statusrespectively.Also applies to: 153-153, 168-168, 308-308
frontend/app/brand/campaigns/create/page.tsx (3)
76-87: Strengthen type safety in array toggling.The type assertion on lines 78-80 could fail if
formData.target_audience[field]is undefined or not an array. While the|| []fallback helps, it only applies after the unsafe cast.Apply this diff to ensure safe array handling:
const toggleTargetAudienceArray = (field: string, value: string) => { - const current = - (formData.target_audience[ - field as keyof typeof formData.target_audience - ] as string[]) || []; + const fieldValue = formData.target_audience[field as keyof typeof formData.target_audience]; + const current = Array.isArray(fieldValue) ? fieldValue : []; updateTargetAudience( field, current.includes(value) ? current.filter((item) => item !== value) : [...current, value] ); };
117-135: Enhance validation for invalid numeric and date inputs.The current validation doesn't guard against invalid user input. For example, if a user types non-numeric text in budget fields,
parseFloatreturnsNaN, and the comparisonNaN > NaNevaluates tofalse, causing invalid data to pass validation. Similarly, invalid date strings can produce unexpected behavior.Apply this diff to add explicit validity checks:
const validateForm = (): boolean => { if (!formData.title.trim()) { setError("Campaign title is required"); return false; } if (formData.budget_min && formData.budget_max) { - if (parseFloat(formData.budget_min) > parseFloat(formData.budget_max)) { + const minBudget = parseFloat(formData.budget_min); + const maxBudget = parseFloat(formData.budget_max); + if (isNaN(minBudget) || isNaN(maxBudget)) { + setError("Budget values must be valid numbers"); + return false; + } + if (minBudget > maxBudget) { setError("Minimum budget cannot be greater than maximum budget"); return false; } } if (formData.starts_at && formData.ends_at) { - if (new Date(formData.starts_at) > new Date(formData.ends_at)) { + const startDate = new Date(formData.starts_at); + const endDate = new Date(formData.ends_at); + if (isNaN(startDate.getTime()) || isNaN(endDate.getTime())) { + setError("Invalid date format"); + return false; + } + if (startDate > endDate) { setError("Start date cannot be after end date"); return false; } } return true; };
192-196: Consider enhancing accessibility with ARIA attributes.The error message display is good, but could be better associated with form fields for screen readers. Additionally, consider adding confirmation before discarding unsaved changes.
For improved accessibility, you could:
- Link error messages to relevant fields using
aria-describedby:<input type="text" value={formData.title} onChange={(e) => updateField("title", e.target.value)} aria-describedby={error ? "form-error" : undefined} aria-invalid={!!error} // ... other props />
- Add an id to the error div:
{error && ( <div id="form-error" role="alert" className="mb-6 rounded-lg bg-red-50 p-4 text-red-600"> {error} </div> )}
- Consider adding a confirmation dialog when canceling with unsaved changes (lines 649-656).
Also applies to: 209-216
backend/app/api/routes/campaigns.py (3)
33-50: Consider adding validation to update schema fields.The
CampaignUpdateschema lacks validation that exists inCampaignCreate. If a client updatesstatuswith an invalid value ortitlewith an empty/oversized string, the database will reject it, but validation should happen at the API layer for better error messages.Consider adding the same validation rules:
class CampaignUpdate(BaseModel): """Schema for updating an existing campaign.""" - title: Optional[str] = None + title: Optional[str] = Field(None, min_length=1, max_length=255) slug: Optional[str] = None short_description: Optional[str] = None description: Optional[str] = None - status: Optional[str] = None + status: Optional[str] = Field(None, pattern="^(draft|active|paused|completed|archived)$") platforms: Optional[List[str]] = None
110-110: Move import to module level.Importing
reinside the function is inefficient and non-idiomatic. Place it with the other imports at the top of the file.+import re from fastapi import APIRouter, HTTPException, Depends, QueryThen remove line 110 inside the function.
233-233: Move logging import to module level.Similar to the
reimport,loggingshould be imported at the top of the file rather than inside the function.+import logging from fastapi import APIRouter, HTTPException, Depends, QueryThen initialize the logger at module level:
logger = logging.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/SQL(1 hunks)backend/app/api/routes/campaigns.py(1 hunks)docs/database/schema-reference.md(1 hunks)frontend/app/brand/campaigns/create/page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/database/schema-reference.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-08T12:24:11.575Z
Learnt from: Saahi30
Repo: AOSSIE-Org/InPactAI PR: 145
File: backend/SQL:98-112
Timestamp: 2025-11-08T12:24:11.575Z
Learning: In the InPactAI repository, the backend/SQL file is a documentation reference that points to schema-reference.md and is not meant to be executed directly. Table ordering and FK constraint issues should not be flagged for this file.
Applied to files:
backend/SQL
📚 Learning: 2025-11-08T12:24:11.575Z
Learnt from: Saahi30
Repo: AOSSIE-Org/InPactAI PR: 145
File: backend/SQL:98-112
Timestamp: 2025-11-08T12:24:11.575Z
Learning: When a SQL file in the backend/SQL directory contains a warning comment indicating it is "for context only" or "not meant to be run," do not flag table ordering or foreign key dependency issues, as the file is for documentation purposes only, not execution.
Applied to files:
backend/SQL
🪛 Ruff (0.14.3)
backend/app/api/routes/campaigns.py
84-84: Abstract raise to an inner function
(TRY301)
92-92: Use explicit conversion flag
Replace with conversion flag
(RUF010)
152-152: Abstract raise to an inner function
(TRY301)
159-159: Use explicit conversion flag
Replace with conversion flag
(RUF010)
171-171: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
172-172: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
227-227: Consider moving this statement to an else block
(TRY300)
263-263: Abstract raise to an inner function
(TRY301)
265-265: Consider moving this statement to an else block
(TRY300)
272-272: Use explicit conversion flag
Replace with conversion flag
(RUF010)
298-298: Abstract raise to an inner function
(TRY301)
305-305: Abstract raise to an inner function
(TRY301)
328-328: Abstract raise to an inner function
(TRY301)
337-337: Use explicit conversion flag
Replace with conversion flag
(RUF010)
361-361: Abstract raise to an inner function
(TRY301)
366-366: Consider moving this statement to an else block
(TRY300)
373-373: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
backend/SQL (1)
1-15: LGTM! Proper idempotent ENUM type definitions.The conditional creation of custom ENUM types addresses the critical issue flagged in previous reviews. The DO block with
IF NOT EXISTSchecks ensures idempotency, and the ENUM values are appropriate for their respective business logic.frontend/app/brand/campaigns/create/page.tsx (4)
1-48: Well-structured component setup.The imports, state initialization, and type usage are clean and appropriate. Using strings for budget and date fields is the correct approach for HTML form inputs.
89-108: Excellent improvement to validation feedback.The previous issue with
alert()has been properly addressed. UsingsetErrorprovides consistent, non-disruptive user feedback that matches the component's design pattern.
137-165: Proper handling of optional fields.The previous issue with empty strings being sent to the API has been correctly addressed. Converting empty strings to
undefinedand parsing numeric values ensures compatibility with FastAPI/Pydantic validation requirements.
386-398: Sensible default for quantity input.The fallback to 1 for invalid or empty quantity values is appropriate for this use case, working in tandem with the
min="1"attribute to ensure valid deliverable quantities.backend/app/api/routes/campaigns.py (1)
210-214: Verify intended budget filter semantics.The review's analysis is mathematically sound. The current implementation filters using containment semantics (campaigns must fit entirely within the user's budget range), while the suggested fix implements overlap semantics (campaigns with any overlapping budget range).
However, the documentation doesn't explicitly specify which behavior is intended. No tests exist to clarify this design decision.
To proceed:
- Confirm with product requirements whether budget filtering should use containment or overlap semantics
- If overlap is intended, apply the suggested fix
- Consider adding tests to document the intended filter behavior
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/app/api/routes/campaigns.py (3)
11-11: Consider adding prefix and tags to the router for better API organization.Adding a prefix and tags will improve API documentation and route organization.
Apply this diff:
-router = APIRouter() +router = APIRouter(prefix="/api/v1", tags=["campaigns"])Note: Adjust the prefix based on how this router is mounted in
main.py.
108-122: Move import to module level.The
import restatement should be at the module level for better performance and readability.Apply this diff at the top of the file:
from datetime import datetime, timezone from app.core.supabase_clients import supabase_anon from uuid import UUID +import reThen remove the import from line 110:
# Generate slug if not provided if not campaign.slug: - import re slug = re.sub(r'[^a-z0-9]+', '-', campaign.title.lower()).strip('-')
123-169: Move import to module level and rename unused loop variable.Two minor issues:
import timeshould be at the module level- The loop variable
attemptis declared but never used in the loop bodyApply these changes:
At the top of the file:
from datetime import datetime, timezone from app.core.supabase_clients import supabase_anon from uuid import UUID +import re +import timeIn the function:
- import time max_attempts = 5 - for attempt in range(max_attempts): + for _attempt in range(max_attempts): try:Note: The slug uniqueness check at lines 116-121 and this retry loop somewhat duplicate effort. The retry loop alone could handle all uniqueness conflicts. Consider simplifying by removing the initial check or document why both are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/campaigns.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
backend/app/api/routes/campaigns.py
84-84: Abstract raise to an inner function
(TRY301)
92-92: Use explicit conversion flag
Replace with conversion flag
(RUF010)
125-125: Loop control variable attempt not used within loop body
Rename unused attempt to _attempt
(B007)
155-155: Abstract raise to an inner function
(TRY301)
167-167: Use explicit conversion flag
Replace with conversion flag
(RUF010)
180-180: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
181-181: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
236-236: Consider moving this statement to an else block
(TRY300)
272-272: Abstract raise to an inner function
(TRY301)
274-274: Consider moving this statement to an else block
(TRY300)
281-281: Use explicit conversion flag
Replace with conversion flag
(RUF010)
307-307: Abstract raise to an inner function
(TRY301)
313-313: Abstract raise to an inner function
(TRY301)
334-334: Abstract raise to an inner function
(TRY301)
343-343: Use explicit conversion flag
Replace with conversion flag
(RUF010)
367-367: Abstract raise to an inner function
(TRY301)
372-372: Consider moving this statement to an else block
(TRY300)
379-379: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (11)
backend/app/api/routes/campaigns.py (11)
14-31: LGTM!The model is well-structured with appropriate validations, proper use of
default_factoryfor mutable defaults, and clear field types.
52-74: LGTM!The response model comprehensively represents the campaign data structure.
76-93: LGTM!The helper function correctly resolves the brand ID with appropriate error handling and exception chaining.
142-149: LGTM!Datetime fields are correctly serialized to ISO format, and
published_atis appropriately set when a campaign is created with active status.
219-223: Verify budget filter logic matches requirements.The current budget filter implementation only returns campaigns whose entire budget range falls within the provided filter range:
budget_minfilter: returns campaigns wherecampaign.budget_min >= filter.budget_minbudget_maxfilter: returns campaigns wherecampaign.budget_max <= filter.budget_maxExample: Filtering with
budget_min=1000andbudget_max=2000will only return campaigns like:
- ✅ Campaign with budget 1000-2000 (within range)
- ✅ Campaign with budget 1200-1800 (within range)
- ❌ Campaign with budget 500-1500 (overlaps but min is outside)
- ❌ Campaign with budget 1500-3000 (overlaps but max is outside)
Most users expect budget filters to find campaigns with overlapping budget ranges instead. The typical pattern would be:
if budget_min is not None: query = query.gte("budget_max", budget_min) # Campaign max >= filter min if budget_max is not None: query = query.lte("budget_min", budget_max) # Campaign min <= filter maxThis would find any campaign whose budget range overlaps with the filter range.
Please verify with the product requirements whether the current "within range" logic is intentional or if "overlapping range" logic is preferred.
238-248: LGTM!Exception handling is well-implemented with server-side logging and generic user-facing error messages that don't expose internal details.
251-282: LGTM!The endpoint correctly fetches a single campaign with proper ownership verification and error handling. (Note: The authentication issue flagged in the create endpoint applies here too.)
302-313: LGTM!Proper use of Pydantic v2
model_dump(exclude_none=True)and good validation to reject empty updates.
315-328: LGTM!Timestamp management is correct with timezone-aware datetimes, defensive serialization checks, and optimized
published_atlogic.
330-344: LGTM!Update execution and error handling are properly implemented.
346-379: LGTM!The delete endpoint correctly verifies ownership before deletion and returns the appropriate 204 status.
| slug: Optional[str] = None | ||
| short_description: Optional[str] = None | ||
| description: Optional[str] = None | ||
| status: Optional[str] = None |
There was a problem hiding this comment.
Add pattern validation to the status field.
The status field in CampaignUpdate lacks the pattern validation present in CampaignCreate. This allows invalid status values to be submitted in update requests, potentially causing database constraint violations or inconsistent data.
Apply this diff:
- status: Optional[str] = None
+ status: Optional[str] = Field(None, pattern="^(draft|active|paused|completed|archived)$")🤖 Prompt for AI Agents
In backend/app/api/routes/campaigns.py around line 39, the CampaignUpdate
model's status field is declared as "status: Optional[str] = None" without the
pattern validation used in CampaignCreate; update this declaration to mirror the
CampaignCreate validation (use Pydantic Field with the same regex/pattern and
any allowed enum values), e.g. replace the plain Optional[str] with
Optional[str] = Field(None, regex="...") or the same constr/Enum used in
CampaignCreate, and ensure you import Field (or the appropriate type) so update
requests are validated against the same status pattern as creates.
| @router.post("/campaigns", response_model=CampaignResponse, status_code=201) | ||
| async def create_campaign(campaign: CampaignCreate, user_id: str = Query(..., description="User ID from authentication")): | ||
| """ | ||
| Create a new campaign for a brand. | ||
| - **user_id**: The authenticated user's ID (should be passed from auth middleware) | ||
| - **campaign**: Campaign details matching the database schema | ||
| """ | ||
| supabase = supabase_anon | ||
|
|
||
| # Get brand ID from user ID | ||
| brand_id = await get_brand_id_from_user(user_id) | ||
|
|
There was a problem hiding this comment.
Critical: Authentication bypass vulnerability.
The user_id is accepted as a query parameter, which can be trivially spoofed by clients. This allows any user to create campaigns for any brand by simply passing a different user_id in the URL.
Authentication should be enforced via FastAPI dependencies that extract the authenticated user from the request context (JWT token, session, etc.). Example:
+from fastapi import Depends
+from app.core.auth import get_current_user # Your auth dependency
+
@router.post("/campaigns", response_model=CampaignResponse, status_code=201)
-async def create_campaign(campaign: CampaignCreate, user_id: str = Query(..., description="User ID from authentication")):
+async def create_campaign(
+ campaign: CampaignCreate,
+ current_user: dict = Depends(get_current_user)
+):
"""
Create a new campaign for a brand.
- - **user_id**: The authenticated user's ID (should be passed from auth middleware)
- **campaign**: Campaign details matching the database schema
"""
supabase = supabase_anon
# Get brand ID from user ID
- brand_id = await get_brand_id_from_user(user_id)
+ brand_id = await get_brand_id_from_user(current_user["id"])This same issue affects all endpoints in this file. Implement proper authentication middleware before deploying to production.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/api/routes/campaigns.py around lines 95 to 107, the endpoint
currently accepts user_id as a Query parameter which allows spoofing; replace
this with a FastAPI authentication dependency (e.g., current_user: User =
Depends(get_current_user_from_token)) and remove the user_id query param so the
handler uses the authenticated user's id; validate that the authenticated user
owns or is allowed to create campaigns for the target brand (return 401/403 when
unauthorized), and apply the same dependency-driven authentication change to
every other endpoint in this file that currently accepts user_id from the
client.
📝 Description
This pull request introduces advanced filtering options to the campaign management system for brands. It enhances both the backend and frontend to support filtering campaigns by platform, budget range, and campaign start/end dates, in addition to existing filters. The frontend UI now includes controls for these filters, providing a more powerful and user-friendly experience for brand users.
🔧 Changes Made
/campaignsendpoint.📷 Screenshots or Visual Changes (if applicable)
✅ Checklist
Summary by CodeRabbit
New Features
Documentation