Skip to content

fix(webhooks): accept null leaves in settings GET response#39

Merged
boorad merged 1 commit into
mainfrom
fix/webhook-null-leaves
May 17, 2026
Merged

fix(webhooks): accept null leaves in settings GET response#39
boorad merged 1 commit into
mainfrom
fix/webhook-null-leaves

Conversation

@boorad

@boorad boorad commented May 17, 2026

Copy link
Copy Markdown
Owner

Summary

GHIN's webhook settings GET response always includes every event key under webhook_url, webhook_data_type, and webhook_enabled, using null as the "unregistered" sentinel rather than omitting the key. This was causing schemaWebhookSettings to fail Zod parsing and ensureRegistered to misreport state. The response schema now accepts string | null | undefined per leaf, while PATCH bodies retain the stricter "optional, no null" shape (use '' to clear a URL).

Changes

  • Split the inner event-map helper into partialEventMapResponse (accepts null) and partialEventMapRequest (rejects null) so GET and PATCH schemas are intentionally asymmetric.
  • Widen normalizeWebhookUrl and describeLeaf to accept null and treat it the same as undefined / (not set).
  • Add a regression test covering a fully-nulled GET response across all six event keys.
  • Add a changeset (patch).

Testing

  • bun run test:run — 285 tests pass
  • ./scripts/code-quality.sh — clean

The new test mocks GHIN returning null for every leaf and asserts ensureRegistered PATCHes with a (not set) reason.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed webhook settings handling to properly recognize when webhooks are not registered. The application now correctly processes webhook configuration states and improves the accuracy of registration status reporting.

Review Change Stack

GHIN's GET /webhook_settings returns every event key on every top-level
field with `null` as the "unregistered" sentinel, not an omitted key.
The previous schema rejected null, causing parse failures and incorrect
`ensureRegistered` change detection. Split the inner schema into
response (accepts null + undefined) and request (optional only) variants
so PATCH bodies still steer callers toward `''` for clearing a URL.
@boorad boorad self-assigned this May 17, 2026
@coderabbitai

coderabbitai Bot commented May 17, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6354b02f-8cce-4c79-9d32-009c33a35aa7

📥 Commits

Reviewing files that changed from the base of the PR and between c505555 and 0ef7508.

📒 Files selected for processing (4)
  • .changeset/webhook-null-leaves.md
  • src/client/ghin/index.test.ts
  • src/client/ghin/index.ts
  • src/client/ghin/models/webhooks/settings.ts

📝 Walkthrough

Walkthrough

This PR extends webhook settings handling to accept null values from GHIN as an unregistered sentinel. The GET response schema now permits null for webhook leaf fields, while PATCH request bodies remain non-nullable. Helper functions and tests are updated to treat null identically to undefined when determining state changes.

Changes

Webhook Null Value Support

Layer / File(s) Summary
Webhook Settings Schema Contracts
src/client/ghin/models/webhooks/settings.ts
New response and request helpers distinguish nullable event-map parsing for GET responses (accepting null|undefined) from PATCH bodies (accepting only undefined), updating the inferred WebhookSettings and WebhookSettingsPatch types.
Webhook URL and Leaf Normalization
src/client/ghin/index.ts
normalizeWebhookUrl and describeLeaf functions are broadened to accept and normalize null using the same "(not set)" semantics as undefined.
Null Webhook Leaf Test Case
src/client/ghin/index.test.ts
New test case verifies ensureRegistered treats null webhook leaves from GHIN as "not set", triggers a PATCH, and reports the URL difference correctly.
Changeset Documentation
.changeset/webhook-null-leaves.md
Changeset entry documents the fix: GHIN includes null leaves for unregistered webhooks, GET schema now accepts null, and PATCH maintains stricter optional-only shapes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Null leaves from GHIN were causing strife,
But now we parse them through to life—
GET accepts them, PATCH stays strict,
A null-safe webhook dance well-licked! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main fix: accepting null values in webhook settings GET responses, which is the core change across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@c505555). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #39   +/-   ##
=======================================
  Coverage        ?   95.73%           
=======================================
  Files           ?       53           
  Lines           ?     2488           
  Branches        ?      423           
=======================================
  Hits            ?     2382           
  Misses          ?      106           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@boorad boorad merged commit 960b405 into main May 17, 2026
7 checks passed
@boorad boorad deleted the fix/webhook-null-leaves branch May 17, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant