Conversation
chore: fix type error from reading cadence from the wrong place
Summary of ChangesHello @ElasticBottle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the platform's SEO capabilities by introducing a robust strategy management system. Users can now interact with AI-generated SEO strategy suggestions directly within the application, choosing to adopt or dismiss them, and modifying their details. This feature is seamlessly integrated into the onboarding process, providing immediate value to new projects. The underlying architecture has been updated with new database schemas and a dedicated AI workflow to support this functionality, alongside refinements to the content publishing pipeline. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fc3bfb091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .handler(async ({ context, input }) => { | ||
| const strategiesResult = await listStrategiesByProjectId({ | ||
| db: context.db, | ||
| projectId: input.projectId, | ||
| }); |
There was a problem hiding this comment.
These routes only pass projectId into the DB lookup, and the middleware only verifies membership in the supplied organization, not that the project belongs to it. That means a user in org A can supply org A’s identifier but a projectId from org B and still retrieve that project’s strategies if they know the ID. Add a project/org ownership check (e.g., join through seoProject or validate projectId against context.organization.id) before calling listStrategiesByProjectId (and similarly for get/update).
Useful? React with 👍 / 👎.
| ALTER TABLE "seo_content_draft" DROP CONSTRAINT "seo_content_draft_base_content_id_seo_content_id_fk"; | ||
| --> statement-breakpoint | ||
| DROP INDEX "seo_content_draft_org_project_status_base_id_idx";--> statement-breakpoint | ||
| ALTER TABLE "seo_content" ADD COLUMN "originating_draft_id" uuid NOT NULL;--> statement-breakpoint |
There was a problem hiding this comment.
This migration adds originating_draft_id as NOT NULL on seo_content without a default or a backfill. On any environment where seo_content already has rows, Postgres will error because existing rows get NULL for the new column. You’ll need to add the column nullable, backfill from existing draft/content relations, then enforce NOT NULL.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new "Strategy" feature, including UI, API, database schema, and AI workflows. It also includes a major and well-executed refactoring of the content draft lifecycle, making drafts long-lived entities. The changes are extensive but appear consistent and well-structured. My review focuses on a few areas for improvement regarding robustness and user experience in the new components.
| <StrategyModifyDialog | ||
| organizationId={organizationId ?? ""} | ||
| projectId={projectId ?? ""} | ||
| strategy={strategy} | ||
| /> |
There was a problem hiding this comment.
The StrategyModifyDialog is being passed organizationId ?? "" and projectId ?? "". If the original IDs are undefined, this will result in empty strings being passed to a component that expects valid identifiers. This can lead to failed API calls. It's safer to conditionally render the StrategyModifyDialog only when both organizationId and projectId are available.
{organizationId && projectId && (
<StrategyModifyDialog
organizationId={organizationId}
projectId={projectId}
strategy={strategy}
/>
)}
| if (!organizationId || !projectId) { | ||
| form.setError("root", { | ||
| message: "Missing organization or project details.", | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The check if (!organizationId || !projectId) is not robust against empty strings. Since the parent component might pass "", this check will evaluate to false, but the subsequent API call will likely fail with an empty organizationIdentifier. You should trim the strings before checking to ensure they are not empty. This also applies to the disabled prop of the save button.
| if (!organizationId || !projectId) { | |
| form.setError("root", { | |
| message: "Missing organization or project details.", | |
| }); | |
| return; | |
| } | |
| if (!organizationId.trim() || !projectId.trim()) { | |
| form.setError("root", { | |
| message: "Missing organization or project details.", | |
| }); | |
| return; | |
| } |
| <Button | ||
| isLoading={isAdopting} | ||
| onClick={() => handleStatusChange("planned")} | ||
| size="sm" | ||
| type="button" | ||
| > | ||
| Adopt Strategy | ||
| </Button> |
There was a problem hiding this comment.
For consistency and better user experience, the "Adopt Strategy" button should be disabled when organizationId or projectId are missing, similar to how the "Dismiss" button is handled. While the handleStatusChange function has guards, disabling the button provides clearer feedback to the user.
<Button
disabled={isAdopting || !organizationId || !projectId}
isLoading={isAdopting}
onClick={() => handleStatusChange("planned")}
size="sm"
type="button"
>
Adopt Strategy
</Button>
| ## Expectations | ||
| - Provide concise, exact values without extra commentary.`; | ||
|
|
||
| const { experimental_output } = await generateText({ |
There was a problem hiding this comment.
The system prompt for the AI has potentially conflicting instructions. It asks the model to "Return the homepage title exactly as provided" and also to "Extract the brand or product name from the title". Since the output schema only expects the extracted name, the instruction to return the full title is unnecessary and could confuse the model. It's better to have the prompt focus solely on the extraction task.
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive strategy management feature to the SEO platform. The changes add database schemas for strategies, phases, snapshots, and associated content, along with corresponding API routes, workflows, and UI components.
Changes:
- Adds new database tables for strategy management (seo_strategy, seo_strategy_phase, seo_strategy_snapshot, etc.)
- Replaces
baseContentIdin content drafts withoriginatingDraftIdin published content - Removes content attribution tables (seo_content_chat, seo_content_contributor)
- Adds strategy suggestions workflow integrated into onboarding
- Implements strategy-related API routes and operations
- Updates content publishing flow to mark drafts as "published" instead of deleting them
Reviewed changes
Copilot reviewed 58 out of 61 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/migrations/0018_square_black_cat.sql | Creates new strategy tables and drops old attribution tables |
| packages/db/migrations/0019_noisy_black_bird.sql | Adds originatingDraftId to seo_content and updates indexes |
| packages/db/src/schema/seo/*.ts | New schemas for strategy, phase, snapshot, and related entities |
| packages/db/src/operations/seo/strategy-operations.ts | CRUD operations for strategies and phases |
| packages/api-seo/src/workflows/strategy-suggestions-workflow.ts | AI workflow to generate strategy suggestions |
| packages/api-seo/src/routes/strategy.ts | REST API for strategy management |
| packages/api-seo/src/lib/content/*.ts | Updated content operations for new model |
| apps/seo/src/routes/_authed/onboarding/*.tsx | UI for strategy insights in onboarding |
| apps/seo/src/routes/_authed/$organizationSlug/$projectSlug/-components/*.tsx | Strategy card and modify dialog UI |
| ALTER TABLE "seo_content_draft" DROP CONSTRAINT "seo_content_draft_base_content_id_seo_content_id_fk"; | ||
| --> statement-breakpoint | ||
| DROP INDEX "seo_content_draft_org_project_status_base_id_idx";--> statement-breakpoint | ||
| ALTER TABLE "seo_content" ADD COLUMN "originating_draft_id" uuid NOT NULL;--> statement-breakpoint |
There was a problem hiding this comment.
Migration 0019 adds a NOT NULL column 'originating_draft_id' to existing 'seo_content' table without providing a default value or a data migration step. This will fail if there are any existing rows in the seo_content table. You need to either:
- Add a default value temporarily
- Make the column nullable initially and backfill data
- Include a data migration step before adding the NOT NULL constraint
| if (!scheduledFor && !hasPublishedSnapshot && updatedDraft.strategyId) { | ||
| const currentPhase = await getCurrentStrategyPhase({ | ||
| db: args.db, | ||
| strategyId: updatedDraft.strategyId, | ||
| }); | ||
| if (!currentPhase.ok) { | ||
| return currentPhase; | ||
| } | ||
| const project = projectResult.value; | ||
|
|
||
| const cadence = project.publishingSettings?.cadence; | ||
| if (cadence) { | ||
| const scheduledItemsResult = await getScheduledItems({ | ||
| db: args.db, | ||
| organizationId: draft.organizationId, | ||
| projectId: draft.projectId, | ||
| const { cadence } = currentPhase.value; |
There was a problem hiding this comment.
The logic assumes that if a draft has a strategyId, there must be a current phase. However, getCurrentStrategyPhase can return an error (line 189 in strategy-operations.ts: "Failed to find strategy phase") if no phase exists. This will prevent scheduling content for strategies that don't yet have phases, which could happen during strategy setup. Consider handling the case where no phase exists yet more gracefully, perhaps by falling back to immediate scheduling or returning a more specific error.
| ALTER TABLE "seo_content_chat" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint | ||
| ALTER TABLE "seo_content_contributor" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint | ||
| DROP TABLE "seo_content_chat" CASCADE;--> statement-breakpoint | ||
| DROP TABLE "seo_content_contributor" CASCADE;--> statement-breakpoint |
There was a problem hiding this comment.
Migration 0018 drops the seo_content_chat and seo_content_contributor tables with CASCADE, which will also drop any foreign key constraints from other tables that reference them. However, the schema changes in content-schema.ts show these relations being removed from the code. Verify that no other production code still references these tables, and ensure the migration order is correct if this runs on a live database with existing data.
chore(api-seo): add more information in logs fix(api-seo): type errors
refactor: unify bunch of duplicated code
1 participant
No description provided.