fix(inbox): review fixes for plugin-inbox (G-P3-1)

Merged
EV
10d ago
+447-86

Plan item: G-P3-1 @gauzy/plugin-inbox (Gauzy ↔ Ever Works integration plan, 04-inbox-and-email.md §2–4, 06-gauzy-implementation-plan.md Phase 3).

Owned by 7dae55c0/gw-gauzy-inbox. Other lanes, please don't push to this branch.

Why this PR exists

G-P3-1 reached develop as commit 805c8ead48 (merged by 3b4d763f0b) through an accidental direct push. It had no PR and no bot review. This PR is that review, done after the fact, plus the fixes it found. The whole plugin is in scope for the bots here: @coderabbitai please review all of packages/plugins/inbox/ and packages/contracts/src/lib/inbox.model.ts, not only this diff.

What I reviewed in 805c8ead48

AreaResult
Flag GAUZY_INBOX_ENABLED (only the exact 'true')OK. isInboxEnabled copies the Ever Connect gate (no imports, warns once on other values). With the flag off, apps/api/src/plugins.ts doesn't load the plugin at all: no route, no entity, no migration. The plugin matrix spec covers it, and also covers it alongside EVER_WORKS_ENABLED.
Entities on both ORMs (MultiORMEntity, TypeORM + MikroORM repositories, TypeOrmModule/MikroOrmModule.forFeature)Bug, fixed (boot): see finding 1. Otherwise OK. Entities are passed as arrays, not factories.
Migrations on Postgres, SQLite and MySQLOK. They are hand-written per dialect and idempotent (probe, then IF NOT EXISTS), with an advisory lock on Postgres. Unique (inboxItemId, userId) and (tenantId, externalSource, externalId). NULLs are distinct on all three databases, which gives the partial uniqueness the plan asks for. Tenant/org/item/user FKs cascade. down drops the tables; the role-grant down is a documented no-op, the same as the platform's other permission migrations. No timestamp collides with another migration (1791000002100..2300).
Permissions and seeded role rowsOK. INBOX_VIEW, INBOX_MAILBOX_MANAGE and INBOX_MAILBOX_READ_ALL are in the contract, in PermissionGroups and in DEFAULT_ROLE_PERMISSIONS. The migration grants ADMIN/SUPER_ADMIN all three and MANAGER/EMPLOYEE INBOX_VIEW, and leaves rows a tenant decided alone.
REST (/api/inbox/items*)TenantPermissionGuard + PermissionGuard + INBOX_VIEW are applied, every answer is Cache-Control: no-store, the DTOs are validated (UUIDs, enums, lengths), and the recipient always comes from the credential. Privacy bug fixed: see finding 2.
GraphQL twinThere is none yet. The plan's GraphQL mirror of the read routes and markInboxItem is still to do (the README says so). It's a follow-up, not part of G-P3-1's scope line.
Tenant/org isolationReads join the caller's own recipient row in the caller's tenant. Hardened: a source's organizationId must belong to the item's tenant (finding 4).
Reply flow (claim → source handler → release on refusal)Bug, fixed: see finding 3.
Source writer (upsertExternal / create) and source-handler registryBug, fixed: see finding 4. The INBOX_ITEM_WRITER / INBOX_SOURCE_HANDLER_REGISTRY tokens come from a @Global() module, so feeders inject them optionally.
Wiring checklist (research/05 §1 and §10)OK. Covered: tsconfig.json path, apps/api/package.json, gated apps/api/src/plugins.ts, the root build lists (build:package:plugins:post*, api*, all*), the api and worker Dockerfiles (both COPY stages and the node_modules copy), the desktop/server/server-api package.json files, build.yml test step, .env.sample/.env.docker/.env.compose. Not needed: the worker plugin list (no queue or cron), the webapp Dockerfile and webpack copy-exclusion (backend only; the UI package comes with G-P3-2), and the egress baselines (no outbound hostnames).
cspell / prettier on the plugin's filesClean, before and after.

Findings fixed here

  1. The API could not boot with GAUZY_INBOX_ENABLED=true (only when the flag is on). The plugin builds with strictNullChecks. Under that setting, a property typed Date | null or ID | null with no explicit column type is emitted as design type Object. TypeORM refuses Object when the data source initialises (DataTypeNotSupportedError). That affected ten columns: dueAt, entityId, taskId, projectId, agentEmployeeId, mailboxId, emailId, readAt, snoozedUntil and deliveredAt. The database suite maps the tables through fixture EntitySchemas, so it never saw this. These columns are now ?: Date / ?: ID, the core convention (still nullable in the tables). A new inbox-entity-metadata.spec.ts reads the real decorators' design types. I checked the emitted JS: 10 design:type Object before, 0 on implicitly typed columns after.
  2. INBOX_MAILBOX_READ_ALL could read any item of the tenant, including questions, approvals and notices addressed personally to other users. The plan's meaning is shared-mailbox admins. It now reads only mailbox items (mailboxId set, so P4 email) of organizations the reader is an active member of (user_organization). It still gets read access only, no actions.
  3. Double delivery of a reply. If the source handler took a reply but recording routed then failed, the catch block released the claim and the item was OPEN again, so a second reply would reach the source a second time. Now only the handler call is inside the release-on-failure block. A failed bookkeeping write is logged and the item stays ANSWERED.
  4. Re-deliveries regressed items. On an existing external identity, upsertExternal rewrote every column with its default. A re-delivered "created" payload therefore reset an answered item to status=OPEN, wiped response, and cleared body, options and links. Now only the fields the source states change; a stated null still clears. Also:
    • organizationId is checked against the tenant (400 otherwise).
    • Recipient ids that are not UUIDs are skipped. Postgres would otherwise refuse the whole IN (…) lookup with a 500.
    • mailboxId and emailId are writable through the writer. This is additive, for the P4 inbound pipeline.
  5. Small fixes:
    • setState checks snoozedUntil !== undefined instead of 'snoozedUntil' in input. A DTO compiled with define-semantics class fields carries the key even when it was not sent.
    • The registry trims the source on get/unregister, as it already does on register.

The writer (create, upsertExternal), the recipient model and the registry are the surface the Ever Works integration and the P4 inbound-email lane build on. They stay backward compatible: every change here is additive or a bug fix.

Verification (owner rule tonight: no local test runs; the full suites run once on develop after all merges)

  • tsc --noEmit, strict, on the plugin sources and on its specs: clean.
  • prettier --check and cspell on every touched file: clean.
  • New and updated cases in inbox.db.spec.ts, which run in CI's "Test the Inbox plugin" step and in the final full run:
    • READ_ALL scope;
    • the claim stays when the routed write fails;
    • a re-delivery keeps an answered item's answer and links;
    • foreign / unknown / malformed organizations are refused;
    • non-UUID recipients are skipped.

🤖 Generated with Claude Code


Summary by cubic

Fixes five review findings in @gauzy/plugin-inbox (a post-hoc review of a change pushed to develop without a PR) and adds a Snoozed tab so snoozed items can be listed before their time.

Bug fixes

  • The API boots with GAUZY_INBOX_ENABLED=true: Date | null / ID | null columns without an explicit type are emitted as design type Object under strictNullChecks, which TypeORM refuses; they are ?: Date / ?: ID now.
  • INBOX_MAILBOX_READ_ALL reads only EMAIL items (P4) of organizations the holder is an active member of, never another user's personal items.
  • A reply the source accepts stays claimed even when recording its routing fails, so a second reply can no longer be delivered twice.
  • A source re-delivery changes only the fields it states: an answered item keeps its answer, links, and status.
  • An item's organizationId must belong to its tenant (empty or unknown is refused); recipient ids that are not UUIDs are skipped.
  • mailboxId and emailId are writable through the writer (additive, for the P4 inbound pipeline).
  • setState checks snoozedUntil !== undefined instead of in; the registry trims on get/unregister too.

New features

  • tab=snoozed lists the caller's items snoozed until a future time (not archived), the only way to reach them before their time.

Test suites added: inbox-entity-metadata.spec.ts reads the real decorators' design types so the boot failure can't return, and inbox.db.spec.ts covers each fix plus the new tab. The README is rewritten within markdownlint (80 columns, heading lock) and documents the new behavior.