Skip to content

Fix #1004: docs: PG LISTEN/NOTIFY migration should be documented in s... - #1014

Merged
namastex888 merged 1 commit into
automagik-dev:mainfrom
JiwaniZakir:fix/1004-docs-pg-listen-notify-migration-should-b
Apr 3, 2026
Merged

namastex888 merged 1 commit into
automagik-dev:mainfrom
JiwaniZakir:fix/1004-docs-pg-listen-notify-migration-should-b

Conversation

@JiwaniZakir

@JiwaniZakir JiwaniZakir commented Apr 3, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1004

Expands the module-level JSDoc in src/lib/mailbox.ts to document the architectural migration from .genie/mailbox/<worker>.json file polling to PG LISTEN/NOTIFY that landed in commit 44a153a3. The previous one-liner (PG LISTEN/NOTIFY triggers instant delivery notification on new inserts) was moved and replaced with a full migration note explaining what was replaced, why (polling latency and fragile cross-process coordination), and how the new system works (AFTER INSERT trigger fires pg_notify('genie_mailbox_delivery', <to_worker>:<message_id>), consumed by subscribeDelivery() via sql.listen, with a 30-second fallback poll for reconnect safety).

  • src/lib/mailbox.ts — module docblock only; no runtime logic touched.

Verified by reviewing the rendered JSDoc output and confirming the comment accurately matches the live implementation of subscribeDelivery() and the mailbox table trigger.


This PR was created with AI assistance (Claude). The changes were reviewed by quality gates and a critic model before submission.

Summary by CodeRabbit

  • Documentation
    • Updated mailbox delivery system documentation to reflect the shift from file-based storage to PostgreSQL-backed delivery signaling, including LISTEN/NOTIFY notifications and fallback polling mechanisms.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@coderabbitai

coderabbitai Bot commented Apr 3, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The mailbox.ts module documentation was updated to include a migration note explaining the architectural shift from file-based mailbox storage to PostgreSQL-backed delivery signaling via LISTEN/NOTIFY, documenting the trigger mechanism, notification payload format, and fallback polling behavior.

Changes

Cohort / File(s) Summary
Documentation Update
src/lib/mailbox.ts
Added migration note documenting the transition from .genie/mailbox/<worker>.json file-based polling to PostgreSQL mailbox table with AFTER INSERT triggers, pg_notify() signaling, and sql.listen() subscription with 30-second fallback polling.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds migration documentation to src/lib/mailbox.ts explaining the shift from file-based polling to PG LISTEN/NOTIFY, including what was replaced, why, and how the new system works. However, it does not mark remaining .genie/mailbox/ references as deprecated or add source-level docs sections as requested in #1004. Add deprecation markers to any remaining .genie/mailbox/ references in source code and consider adding a dedicated source-level docs section on messaging architecture.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references issue #1004 and accurately describes the main change: documentation of the PG LISTEN/NOTIFY migration in source code.
Out of Scope Changes check ✅ Passed All changes are limited to updating the module-level JSDoc in src/lib/mailbox.ts to document the PG LISTEN/NOTIFY migration, which is directly aligned with issue #1004 requirements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/mailbox.ts`:
- Line 13: Fix the grammar in the mailbox comment: replace the phrase "A `AFTER
INSERT` trigger" with "An `AFTER INSERT` trigger" so the article matches the
vowel sound before the quoted `AFTER INSERT` trigger text; update the comment
containing "AFTER INSERT" to use "An" accordingly.
🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro

Run ID: 1704324a-42b2-40d7-bcd2-55e3c14c7eda

📥 Commits

Reviewing files that changed from the base of the PR and between de1da2c and 59bbcf5.

📒 Files selected for processing (1)
  • src/lib/mailbox.ts

Comment thread src/lib/mailbox.ts
*
* The current implementation replaced file-based polling with PostgreSQL:
* - Messages are persisted to the `mailbox` table (durable, queryable).
* - A `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Grammar: use "An" before vowel sound.

"A AFTER INSERT trigger" should be "An AFTER INSERT trigger".

📝 Proposed fix
- *  - A `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)`
+ *  - An `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* - A `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)`
* - An `AFTER INSERT` trigger fires `pg_notify('genie_mailbox_delivery', …)`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/mailbox.ts` at line 13, Fix the grammar in the mailbox comment:
replace the phrase "A `AFTER INSERT` trigger" with "An `AFTER INSERT` trigger"
so the article matches the vowel sound before the quoted `AFTER INSERT` trigger
text; update the comment containing "AFTER INSERT" to use "An" accordingly.

@namastex888
namastex888 merged commit ad9772b into automagik-dev:main Apr 3, 2026
2 checks passed
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.

docs: PG LISTEN/NOTIFY migration should be documented in source

2 participants