fix(github): keep a webhook delivery alive when link metadata will not parse - #1526
Conversation
…t parse An external link's metadata is a JSON string in the database, so a row written by an older version, or a truncated one, is not something the delivery can do anything about. On the GitHub side four handlers called `JSON.parse` on it bare, so the throw escaped the handler and the whole delivery failed, leaving the task unsynced with no record of why. The Gitea handlers already warn and carry on with an empty object. This is the same behaviour, in one helper, since the GitHub side needs it in four places. `issue-closed` also stopped at the first matching integration, so a repository connected to two projects only moved one task. Its Gitea twin and `issue-opened` on this side both walk all of them. `pull-request-closed` returns early on both sides, so that one is left as it is.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGitHub webhook handlers now use ChangesGitHub metadata handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
PR Summary by QodoFix GitHub webhooks: guard link metadata parsing and process all integrations
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/api/plugins/github/utils/parse-link-metadata.test.ts (1)
11-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Arrange-Act-Assert in each test.
Separate setup, the
parseLinkMetadatacall, and assertions. This makes each tested behavior explicit.As per coding guidelines,
**/*.test.{ts,tsx,js}requires Arrange-Act-Assert test structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/api/plugins/github/utils/parse-link-metadata.test.ts` around lines 11 - 36, The tests in parse-link-metadata.test.ts currently combine setup, parseLinkMetadata invocation, and assertions. Restructure each test into explicit Arrange, Act, and Assert phases: prepare inputs and spies first, call parseLinkMetadata once in the Act phase, then perform all expectations on the captured result and warning calls.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/plugins/github/utils/parse-link-metadata.ts`:
- Around line 17-21: Update the warning in the GitHub link metadata parsing flow
to remove the raw metadata payload from the log. In the relevant parse-link
metadata function, log only externalLinkId, source, and a classified error
value, preserving the existing failure warning without exposing user-provided
content.
- Line 9: Update parseLinkMetadata() to return a typed metadata shape that
includes lastSync.title and lastSync.description, using a shared schema or a
generic type. Update each caller to provide or narrow that type before assigning
these fields, while preserving the parser’s existing behavior.
- Line 15: Update the JSON parsing logic in the metadata parser to return an
empty object unless the parsed value is a non-null, non-array object; preserve
valid object metadata unchanged. Add coverage for null, arrays, and primitive
JSON values.
---
Nitpick comments:
In `@tests/api/plugins/github/utils/parse-link-metadata.test.ts`:
- Around line 11-36: The tests in parse-link-metadata.test.ts currently combine
setup, parseLinkMetadata invocation, and assertions. Restructure each test into
explicit Arrange, Act, and Assert phases: prepare inputs and spies first, call
parseLinkMetadata once in the Act phase, then perform all expectations on the
captured result and warning calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a649ef0-84c1-4387-82b9-f3ca47285d55
📒 Files selected for processing (5)
apps/api/src/plugins/github/utils/parse-link-metadata.tsapps/api/src/plugins/github/webhooks/issue-closed.tsapps/api/src/plugins/github/webhooks/issue-edited.tsapps/api/src/plugins/github/webhooks/pull-request-closed.tstests/api/plugins/github/utils/parse-link-metadata.test.ts
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
`JSON.parse` returned `any`, so reading `metadata.lastSync.title` off it went unchecked. The helper hands back a typed value, which made `tsc` point at four reads that were never verified. `parseLinkMetadata` takes the shape from the caller now, defaulting to the untyped record for the handlers that only spread it forward, and the edit handler declares what it looks for. Every field is optional, since a row may predate any of them. A stamp with no timestamp used to become an Invalid Date, whose NaN failed the recency comparison; the epoch fails it the same way, so the branch taken does not change.
|
The typecheck was right to fail and I had not run it before opening this, only vitest and biome. Fixed in
One behavioural note, since it is the kind of thing worth stating rather than hiding in a diff: a stamp with no
|
… an object `JSON.parse` answers `null` for the row `null`, and a string for `"kaneo"`. The helper handed either one back as metadata, so a caller reading `metadata.createdFrom` off it threw inside the webhook, which is the crash this helper exists to prevent. A parsed value that is not a plain object now takes the same exit as a row that will not parse. Arrays go with them: nothing here writes one, and spreading it forward would turn its indices into keys. The warning no longer carries the row itself. A row can hold a task description synced from Kaneo, and the link id is enough to find it by.
|
Both bots found the same real gap, and it was in the part of this PR that was supposed to close it. Fixed in
The premise of the helper is a row written by an older version or truncated, so a row holding I also took the point about the log. The warning no longer carries the row: Checked the tests catch it rather than just pass. Reverting the guard alone leaves the three tests that were already here green and fails both new ones:
|
Follow-up to the two things I flagged while reviewing #1518 rather than folding them into that diff.
The metadata parse
externalLink.metadatais a JSON string in the database. A row written by an older version, or truncated, is not something the incoming delivery can do anything about, but four handlers on the GitHub side calledJSON.parseon it bare:webhooks/issue-closed.tswebhooks/issue-edited.tswebhooks/pull-request-closed.tsThe throw escapes the handler, so the whole webhook delivery fails and the task is left unsynced with nothing in the log explaining why.
The Gitea handlers already do this properly.
gitea/webhooks/issue-closed.tsandgitea/webhooks/issue-reopened.tsparse inside atry, warn with the external link id, and carry on with an empty object. This does the same, in one helper, since the GitHub side needs it in four places rather than one.The early return
issue-closedreturned after the first matching integration, so a repository connected to two projects moved one task to Done and left the other behind. Its Gitea twin walks all of them, and so doesissue-openedon this side, so this looked like the odd one out rather than a decision.pull-request-closedreturns early on both sides, so I left that one alone. If the intent there is also to process every integration, that is a separate change and I would rather ask than assume.What I deliberately did not touch
JSON.parse(integration.config)is still bare inissue-opened,pull-request-opened,pushandpull-request-closed. The Gitea side guards those too, but config is written by the app rather than arriving from outside, so the failure story is much weaker and it did not feel like it belonged in the same diff. Happy to send it separately if you want the parity.Tests
tests/api/plugins/github/utils/parse-link-metadata.test.ts, three cases: a well formed row, an absent value, and a truncated one where it warns and returns an empty object instead of throwing.The webhook handlers themselves have no tests in the repo today, since they reach the database, so the coverage here is on the helper. Worth saying plainly rather than implying more than there is.
npx vitest run tests/api/plugins/githubgives 7 files and 38 tests passing, andbiome check --error-on-warningsis clean on the five files touched.Summary by CodeRabbit
Bug Fixes
Tests
Sequencing with #1518
#1518 adds
webhooks/issue-reopened.ts, which does this same parse with its own inlinetry, written before this helper existed. The two do not touch the same files so they will not conflict, but if both land the repo ends up with five parses where four go through the helper and one does not.Whichever merges second, I will send a one line follow-up moving
issue-reopenedonto the helper. Say the word if you would rather I fold it into one of them now instead.