fix(inbox): review fixes for plugin-inbox (G-P3-1)
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
| Area | Result |
|---|---|
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 MySQL | OK. 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 rows | OK. 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 twin | There 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 isolation | Reads 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 registry | Bug, 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 files | Clean, before and after. |
Findings fixed here
- The API could not boot with
GAUZY_INBOX_ENABLED=true(only when the flag is on). The plugin builds withstrictNullChecks. Under that setting, a property typedDate | nullorID | nullwith no explicit columntypeis emitted as design typeObject. TypeORM refusesObjectwhen the data source initialises (DataTypeNotSupportedError). That affected ten columns:dueAt,entityId,taskId,projectId,agentEmployeeId,mailboxId,emailId,readAt,snoozedUntilanddeliveredAt. The database suite maps the tables through fixtureEntitySchemas, so it never saw this. These columns are now?: Date/?: ID, the core convention (still nullable in the tables). A newinbox-entity-metadata.spec.tsreads the real decorators' design types. I checked the emitted JS: 10design:type Objectbefore, 0 on implicitly typed columns after. INBOX_MAILBOX_READ_ALLcould 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 (mailboxIdset, so P4 email) of organizations the reader is an active member of (user_organization). It still gets read access only, no actions.- Double delivery of a reply. If the source handler took a reply but recording
routedthen 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. - Re-deliveries regressed items. On an existing external identity,
upsertExternalrewrote every column with its default. A re-delivered "created" payload therefore reset an answered item tostatus=OPEN, wipedresponse, and cleared body, options and links. Now only the fields the source states change; a statednullstill clears. Also:organizationIdis checked against the tenant (400otherwise).- Recipient ids that are not UUIDs are skipped. Postgres would otherwise refuse the whole
IN (…)lookup with a 500. mailboxIdandemailIdare writable through the writer. This is additive, for the P4 inbound pipeline.
- Small fixes:
setStatecheckssnoozedUntil !== undefinedinstead 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 onregister.
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 --checkandcspellon 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
routedwrite 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 | nullcolumns without an explicit type are emitted as design typeObjectunderstrictNullChecks, which TypeORM refuses; they are?: Date/?: IDnow. INBOX_MAILBOX_READ_ALLreads 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
organizationIdmust belong to its tenant (empty or unknown is refused); recipient ids that are not UUIDs are skipped. mailboxIdandemailIdare writable through the writer (additive, for the P4 inbound pipeline).setStatecheckssnoozedUntil !== undefinedinstead ofin; the registry trims onget/unregistertoo.
New features
tab=snoozedlists 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.