prg/main ahead 20260622 - #25
Conversation
Greptile SummaryThis PR introduces a large-scale "proactive pipeline" feature: a signal ingestion and clustering system that ingests GitHub/Discord/portal events, classifies them into customer needs, and generates draft notification/changelog proposals for human review before delivery. It also adds project-scoped public portals, changelog cover images, scheduled publishing, a tag management system, and an anonymous portal voting identity.
Confidence Score: 3/5The core ingest and dedup paths work, but two logic bugs in newly added code need fixing before the pipeline produces reliable data. Two concrete logic defects are introduced in this PR. First, the pipeline scheduler in amendSourceIngest.ts fires with a permanently-undefined sourceEventId, so all evidence rows created through that path are disconnected from the source event record — the by_sourceEvent index is never populated for this flow, and the duplicate-evidence guard is silently skipped. Second, tags.ts has a case-sensitivity split: create deduplicates case-insensitively, while update uses an exact-case index, allowing 'Export' and 'export' to coexist as separate tags. Both are in newly written code on active paths. packages/backend/convex/amendSourceIngest.ts (pipeline scheduler placement), packages/backend/convex/tags.ts (update clash check case sensitivity)
|
| Filename | Overview |
|---|---|
| packages/backend/convex/amendSourceIngest.ts | Pipeline scheduler call placed before sourceEventId is assigned — evidence rows created via this path will always have undefined sourceEventId. |
| packages/backend/convex/pipeline.ts | New proactive signal pipeline — dedup logic is sound but the action wrapper is unnecessary overhead, and the source event ID linkage issue originates here. |
| packages/backend/convex/tags.ts | New tag CRUD mutations — create uses case-insensitive client-side dedup while update uses a case-sensitive index, allowing near-duplicate tag names. |
| apps/web/src/lib/sanitize-portal-html.ts | New regex-based sanitizer for changelog HTML — correctly applied via PortalProse, but JS_URI_ATTR misses formaction/srcset/poster attributes. |
| packages/backend/convex/drafts.ts | New draft proposal flow with approve/reject/update — safetyStrip redaction and gated delivery queue look correct. |
| packages/backend/convex/ingest.ts | New HTTP ingest endpoints for GitHub/Discord webhooks — both verify signatures/tokens before processing. |
| packages/backend/convex/changelogScheduler.ts | New scheduled changelog publisher cron-driven every 5 minutes — correctly notifies subscribers on publish. |
| packages/backend/convex/schemaProactiveTables.ts | New schema tables for the proactive pipeline — indexes look well-structured for the query patterns used. |
| packages/backend/convex/amendPortalReadHandlers.ts | Portal now resolves project-scoped portals first — take(2)/length===1 collision guard is correct. |
| packages/backend/convex/amendContentMutationHandlers.ts | New publish/schedule handlers for changelog — subscriber notification queue is correctly gated by notifySubscribers flag. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant W as Webhook / Portal
participant I as ingest.ts
participant SI as amendSourceIngest
participant S as ctx.scheduler
participant P as pipeline.processEvent
participant CM as pipeline.commitProcessedEvent
participant N as needs table
participant E as evidence table
participant D as draftProposals
W->>I: POST /ingest/githubWebhook
I->>I: verifyGitHubSignature / verifyApiToken
I->>SI: trustedIngestSourceEvent (mutation)
SI->>S: "runAfter(0, processEvent, sourceEventId=undefined)"
SI->>SI: create sourceEvent, notification, reviewItem
S-->>P: processEvent (action)
P->>CM: commitProcessedEvent (mutation)
CM->>CM: dedup check via proactivePipelineEvents
CM->>N: findCompatibleNeed or insert new need
CM->>E: "insert evidence (sourceEventId=undefined)"
CM->>CM: recomputeNeedProof
Note over D: Human review gate
CM-->>D: (agent.ts) insert draftProposal on ship link
D->>D: approve to queueNotificationDeliveries or queueChangelogReview
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant W as Webhook / Portal
participant I as ingest.ts
participant SI as amendSourceIngest
participant S as ctx.scheduler
participant P as pipeline.processEvent
participant CM as pipeline.commitProcessedEvent
participant N as needs table
participant E as evidence table
participant D as draftProposals
W->>I: POST /ingest/githubWebhook
I->>I: verifyGitHubSignature / verifyApiToken
I->>SI: trustedIngestSourceEvent (mutation)
SI->>S: "runAfter(0, processEvent, sourceEventId=undefined)"
SI->>SI: create sourceEvent, notification, reviewItem
S-->>P: processEvent (action)
P->>CM: commitProcessedEvent (mutation)
CM->>CM: dedup check via proactivePipelineEvents
CM->>N: findCompatibleNeed or insert new need
CM->>E: "insert evidence (sourceEventId=undefined)"
CM->>CM: recomputeNeedProof
Note over D: Human review gate
CM-->>D: (agent.ts) insert draftProposal on ship link
D->>D: approve to queueNotificationDeliveries or queueChangelogReview
Reviews (1): Last reviewed commit: "fix(proactive): handle scheduled publish..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| await ctx.scheduler.runAfter(0, internal.pipeline.processEvent, { | ||
| workspaceId: workspace._id, | ||
| sourceEventId, | ||
| externalId: args.externalId, | ||
| text: [args.title, args.labels?.join(" "), args.milestone].filter(Boolean).join("\n"), | ||
| title: args.title, | ||
| author: args.author, | ||
| url: args.url, | ||
| provider, | ||
| labels: args.labels ?? [], | ||
| }); |
There was a problem hiding this comment.
sourceEventId is always undefined when the pipeline is scheduled
The scheduler call at line 82 is placed immediately after let sourceEventId: Id<"sourceEvents"> | undefined; is declared (line 80), before the rest of trustedIngestSourceEventHandler ever assigns it. The variable is undefined at the call site every time. As a result, every evidence row created by the pipeline via this path will have sourceEventId: undefined, making the by_sourceEvent index unusable for pipeline-created evidence — and the existing-evidence dedup check in commitProcessedEvent skips the index lookup entirely (it guards with if (args.sourceEventId)), so the safeguard that prevents duplicate evidence rows is silently disabled for this call site.
| ) | ||
| .first(); | ||
| if (clash && clash._id !== tag._id) throw new Error("A tag with that name already exists."); | ||
| } | ||
|
|
||
| if (nextName !== tag.name) { | ||
| await rewriteTagAcrossEntries(ctx, workspace._id, tag.name, nextName); | ||
| } | ||
|
|
||
| await ctx.db.patch(tag._id, { name: nextName, color: nextColor, updatedAt: Date.now() }); | ||
| return { id: tag._id, name: nextName, color: nextColor }; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
create and update duplicate checks
create collects all workspace tags and does a toLowerCase() comparison, so "Export" and "export" are treated as duplicates. update, however, uses the by_workspace_and_name index with the exact string (nextName), which is a case-sensitive comparison. If a workspace already has a tag "Export", renaming a different tag to "export" passes the clash check (the index returns no result) and produces two near-identical tags that the UI will treat as distinct. The by_workspace_and_name check in update should either use a secondary case-folded lookup, or the create path should also rely on the index with the same casing rule.
| /[\s/]+(?:href|src|xlink:href)\s*=\s*(?:"\s*(?:javascript|data|vbscript):[^"]*"|'\s*(?:javascript|data|vbscript):[^']*'|\s*(?:javascript|data|vbscript):[^\s>]+)/gi; | ||
|
|
||
| export function sanitizePortalHtml(html: string | null | undefined): string { |
There was a problem hiding this comment.
JS_URI_ATTR only covers href, src, and xlink:href — srcset, formaction, and poster are unchecked
Several other HTML attributes can hold URLs that some browsers will load or execute: srcset (images), formaction (buttons, bypasses stripped <form>), and poster (video). A <button formaction="javascript:..."> in authored content would survive the current regexes because the DANGEROUS_VOID strip removes <form> but not <button> or <input>. Given the semi-trusted author model noted in the file's own comment, this is low-risk in practice, but extending JS_URI_ATTR to include srcset|formaction|poster|action would close the gap without adding complexity.
| externalId: v.string(), | ||
| text: v.string(), | ||
| title: v.optional(v.string()), | ||
| author: v.optional(v.string()), | ||
| url: v.optional(v.string()), | ||
| provider: v.optional(v.string()), | ||
| labels: v.optional(v.array(v.string())), | ||
| email: v.optional(v.string()), | ||
| accountId: v.optional(v.string()), | ||
| }, | ||
| returns: v.object({ ok: v.literal(true) }), | ||
| handler: async (ctx, args) => { | ||
| await ctx.runMutation(internal.pipeline.commitProcessedEvent, { | ||
| ...args, | ||
| dedupeKey: `${args.workspaceId}:${args.externalId}`, | ||
| }); | ||
| return { ok: true as const }; | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
processEvent internalAction adds an async round-trip with no benefit
The processEvent handler's only body is await ctx.runMutation(internal.pipeline.commitProcessedEvent, { ...args, dedupeKey: ... }). An internalAction in Convex is warranted when you need to call external APIs or use capabilities unavailable in a mutation. Here there are none — all logic is in commitProcessedEvent. The extra action hop adds one more async step and one more potential point of failure in the scheduler queue. Moving the dedupeKey construction into commitProcessedEvent and calling it directly from ctx.scheduler.runAfter (as an internalMutation) would simplify the call graph without changing behaviour.
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
Automated PR - Greptile CLI review workflow