fix(insights): apply the exact goal and funnel repair shown to users - #724
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 ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Architecture diagram
sequenceDiagram
participant Agent as Insights Agent
participant Shared as Shared Insights Types
participant RPC as Insights RPC Router
participant DB as PostgreSQL
participant Queue as Redis Queue
Note over Agent,Queue: Executable insight repair flow with definition validation
Agent->>Shared: Build execution.changes (target/type/filters/steps)
Shared-->>Agent: Parse & validate via insightDefinitionEditChangesSchema
Agent->>Shared: validateDefinitionRecommendation
alt Valid executable repair
Shared-->>Agent: No error (target/type/steps/filters present)
else Cosmetic-only patch (name/description)
Shared-->>Agent: Error: "name or description change alone is not a repair"
end
Agent->>Shared: Generate action text via describeInsightDefinitionAction
Shared-->>Agent: Human-readable action from actual payload
Agent-->>RPC: act outcome with execution.changes
RPC->>RPC: applyInsightAction
RPC->>Shared: insightDefinitionEditError(entityType, changes)
alt Wrong-entity fields (steps on goal, target on funnel)
Shared-->>RPC: Error
RPC-->>Agent: REJECT (BAD_REQUEST)
end
RPC->>DB: Fetch current definition (goal or funnel)
DB-->>RPC: Current definition with all measurable fields
RPC->>RPC: Build merged changes (null = keep existing)
alt No-op patch (definition unchanged)
RPC-->>Agent: REJECT "does not change the definition"
else Definition actually changes
RPC->>DB: Apply full changes (target/type/filters or steps/filters)
DB->>DB: Update definition in transaction
end
RPC->>Queue: Queue verification recheck job
Queue-->>RPC: Job queued with replyId
RPC-->>Agent: Success (application complete)
Note over Agent,Queue: Goal and funnel paths share the same validation boundary
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
The latest updates on your projects. Learn more about Unkey Deploy
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
izadoesdev
left a comment
izadoesdev
left a comment
There was a problem hiding this comment.
Reviewed the full repair path and prior automated findings. Apply rejects loss, addition, or change of stored funnel step conditions, compares measurement-relevant fields while excluding step names, and preserves existing conditions during actual target repairs. Cosmetic goal patches are checked against the current definition. Changed test blocks use repository formatting.
Validation: 20 real PostgreSQL/Redis router integration tests passed, including three new rejection cases; 230 insight tests, 22 shared tests, root lint and all 33 type/build tasks passed.
Remaining limitation: the agent can propose a patch that Apply rejects against the current definition. Earlier proposal validation is a follow-up quality improvement.
Greptile SummaryThis PR extends executable insight repairs so the displayed action and applied transaction use the same goal or funnel measurement patch.
Confidence Score: 4/5The behavior appears sound, but the explicit repository typing requirement must be satisfied before merging. The executable repair and Apply flows are consistently validated and tested; the only accepted issue is the newly introduced unknown-typed funnel condition schema. Files Needing Attention: packages/shared/src/insights.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Insight agent proposes repair] --> B[Validate entity-specific edit fields]
B --> C[Derive displayed action from executable payload]
C --> D[Persist insight observation]
D --> E[User selects Apply]
E --> F[Reparse action and check staleness]
F --> G[Lock goal or funnel definition]
G --> H[Reject wrong-entity or no-op patch]
H --> I[Apply exact definition changes]
I --> J[Queue verification recheck]
Reviews (1): Last reviewed commit: "fix(insights): preserve funnel condition..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type: z.enum(["PAGE_VIEW", "EVENT", "CUSTOM"]), | ||
| conditions: z.record(z.string(), z.unknown()).optional(), | ||
| }) | ||
| ) |
There was a problem hiding this comment.
The new funnel-step contract defines condition values with z.unknown(). This violates the repository directive to avoid unknown and use explicit types. Define the supported serializable value shape or reuse the existing explicit condition type; this repository requirement must be satisfied before merging.
Context Used: Basic guidelines for the project so vibe coders do... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
A generated insight could promise to repair a goal target while Apply only changed its name and description. Executable edits now carry goal target/type/filters or complete funnel steps/filters through the existing transaction, and the displayed action is derived from the actual payload. New agent repairs cannot be cosmetic renames.
Apply rejects wrong-entity fields, stale definitions, and no-op patches before queuing verification. Historical metadata edits remain readable and applicable when they actually change the definition. Minimal target-only patches do not require redundant metadata fields. No schema migration is required.
Validation: 17 API integration tests against disposable PostgreSQL/Redis, 230 insight tests, 22 shared-contract tests, root lint, all 33 type/build tasks, and the full pre-push suite (27 tasks) passed.
Scope: executable repair contract, agent validation, and Apply boundary. Based directly on staging; no PR dependency. The separate evidence-quality slice also touches agent.ts and shared insights types in different sections, so review the integration after rebasing. AI-assisted implementation; maintainer contribution.
The final commit d3f9b46 also passed all 17 insight router integration tests in CI (run 33971762151, job 101321407873), including the minimal target patch and cosmetic-edit rejection. The local rerun had stalled when Docker stopped responding; CI closes that validation gap. Docker was not restarted.