Skip to content

refactor: remove legacy Twist/migration framing#1

Merged
amix merged 4 commits into
amix/ft.bootstrap-comms-sdkfrom
amix/remove-legacy-references
May 20, 2026
Merged

refactor: remove legacy Twist/migration framing#1
amix merged 4 commits into
amix/ft.bootstrap-comms-sdkfrom
amix/remove-legacy-references

Conversation

@amix
Copy link
Copy Markdown
Member

@amix amix commented May 20, 2026

Context

This SDK was bootstrapped from twist-sdk-typescript; the bootstrap commit
carried over a lot of legacy framing ("as of the UUIDv7 migration", "what's
gone vs. legacy Twist", Comms_API_changes.md references, Twist branding in
the Docusaurus site). Treating it as a clean hard fork makes the docs read
like a first-class SDK and stops the migration narrative from rotting in
place.

What was changed

  • README: dropped the "IDs are base58-encoded UUIDv7" and "What's gone vs. legacy Twist" sections; rewrote the creation guidance as plain SDK doc.
  • Website (docusaurus.config.ts, docs/index.md, docs/authorization.md): renamed Twist*Comms* everywhere, switched logo/social-card to the already-present Todoist assets, removed twist-logo.png.
  • Source comments: stripped UUIDv7-migration framing, Comms_API_changes.md references, and "as of the Todoist-id migration" prose from every client docstring and from src/types/{entities,requests,enums}.ts, src/utils/{uuidv7,url-helpers}.ts, and test-default comments.
  • Also includes incidental refactor: StatusOkSchema / StatusOk hoisted into src/types/entities.ts, per-client XListSchema = z.array(XSchema) helpers, and add-comment-helper.ts combines both groups / directGroupMentions marker offenses into a single error.

Out of scope

  • Did not rename internal symbol names in src/utils/uuidv7.ts (e.g. encodeUuidToBase58, UuidV7Error) — these describe what the code does, not migration history, and renaming the public exports would break the API.
  • inbox-client.ts still uses inline z.array(InboxThreadSchema) rather than a hoisted InboxThreadListSchema; mirroring the rest of the codebase can be a follow-up.

Treat this SDK as a clean hard fork: drop README/website Twist branding
and strip "as of the UUIDv7 migration" / "what's gone vs. legacy Twist" /
`Comms_API_changes.md` references from docstrings and comments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix amix requested a review from scottlovegrove May 20, 2026 14:08
@amix amix added the 👀 Show PR PR must be reviewed before or after merging label May 20, 2026
@amix amix marked this pull request as ready for review May 20, 2026 14:08
Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

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

This PR does a great job cleaning up the legacy Twist and migration framing to help the SDK stand on its own as a first-class library. The extensive documentation updates and incidental refactoring significantly improve the overall clarity and maintainability of the codebase. To wrap things up, there are a few adjustments needed, including exporting the newly introduced list schemas, tightening up the StatusOk schema and creation ID documentation to accurately reflect their strict constraints, and adding targeted tests to cover the combined error branch and parameter serialization.

Share FeedbackReview Logs

Comment thread src/clients/channels-client.ts Outdated
Comment thread src/clients/comments-client.ts Outdated
Comment thread src/clients/conversation-messages-client.ts Outdated
Comment thread src/clients/conversations-client.ts Outdated
Comment thread src/clients/groups-client.ts Outdated
Comment thread README.md
Comment thread src/types/entities.ts Outdated
Comment thread src/clients/add-comment-helper.ts
Comment thread src/clients/comments-client.ts
Comment thread src/clients/conversation-messages-client.ts
Comment thread website/static/img/twist-logo.png Outdated
Applied all 12 P2 review comments:

- Export ChannelListSchema / CommentListSchema / ConversationListSchema /
  ConversationMessageListSchema / GroupListSchema / ThreadListSchema, plus
  the workspaces.getPublicChannels list schema. Matches the project
  convention in AGENTS.md ("all Zod schemas should be exported").
- Tighten StatusOkSchema to `z.object({ status: z.literal('ok') })` so a
  backend regression surfaces as a parse error instead of being silently
  typed away.
- README: keep the UUIDv7/base58 requirement explicit on caller-supplied
  ids so the documented contract matches resolveCreateId's validation.
- Move EVERYONE / EVERYONE_IN_THREAD / GROUP_ID_MARKERS / GroupId out of
  ./entities and into ./enums to break a circular import: ./entities
  imports ./enums for USER_TYPES/WORKSPACE_PLANS, and ./enums was
  importing ./entities for the marker constants. At init time EVERYONE
  was undefined when NOTIFY_AUDIENCE_GROUP_IDS / SENTINEL_GROUP_IDS were
  built, so the marker-rejection guard was dead. Caught by the new
  add-comment-helper tests.
- Add focused regression tests:
  - src/clients/add-comment-helper.test.ts — markers in groups,
    markers in directGroupMentions, both fields at once, and the
    notifyAudience translation.
  - src/clients/comments-client.test.ts — pins
    thread_id / newer_than_ts / older_than_ts / comment_id on the wire.
  - src/clients/conversation-messages-client.test.ts — pins
    conversation_id / newer_than_ts / older_than_ts / direct_mentions /
    direct_group_mentions on the wire.

Test count: 104 -> 115. Type-check, lint, and tests all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix
Copy link
Copy Markdown
Member Author

amix commented May 20, 2026

All 12 review comments are addressed in 480fd0f. Summary:

Comments Fix
1–7 (*ListSchema exports) All six list schemas + the workspaces.getPublicChannels one are now export const ....
8 (README claims wider contract than resolveCreateId honors) README now explicitly says caller-supplied id must be a base58-encoded UUIDv7.
9 (StatusOkSchema looser than its doc) Narrowed to z.object({ status: z.literal('ok') }).
10 (combined-error branch untested) New src/clients/add-comment-helper.test.ts covers markers in groups only, in directGroupMentions only, in both at once, plus the notifyAudience translation. The test surfaced a real bug: a circular import (entitiesenums) was leaving the marker constants undefined at init time, so the rejection guard was dead. Fixed by moving EVERYONE / EVERYONE_IN_THREAD / GROUP_ID_MARKERS / GroupId into ./enums.
11 (comments-client rename untested) New src/clients/comments-client.test.ts pins thread_id, newer_than_ts, older_than_ts, and comment_id on the wire (GET + POST + batch descriptor).
12 (conversation-messages-client rename untested) New src/clients/conversation-messages-client.test.ts pins conversation_id, newer_than_ts, older_than_ts, direct_mentions, and direct_group_mentions on the wire (GET + POST + batch descriptor).

104 → 115 tests, type-check + lint + format all green.

amix and others added 2 commits May 20, 2026 18:40
The CI build (tsconfig.cjs.json) emits, not just type-checks, and was
narrowing `let capturedUrl: URL | null = null` to `never` after the
null guard because the only writes happen inside an MSW closure that
TS doesn't see. Locally `tsc --noEmit` didn't trip on the same code.

Switch the captured-request pattern to a `URL[]` / `Record<string,
unknown>[]` and pop the first entry. Avoids the narrowing dead end and
the test reads cleaner.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Smoke-tested the SDK against comms.staging.todoist.com / workspace 48121
and found three real schema-vs-reality mismatches:

1. Workspace.plan: the WORKSPACE_PLANS enum (`free`|`unlimited`) was
   incomplete — staging returns `business`. Widened to `z.string()` and
   added `business` to the const array. Plans evolve faster than SDK
   releases so the runtime check was the wrong shape.

2. Workspace.imageId / defaultConversation: I had renamed `avatar_id` →
   `imageId` on Workspace, but the `name → fullName` / `avatar_id →
   imageId` rename in `Comms_API_changes.md` PR #125 applies to User
   only. Reverted Workspace back to `avatarId`. Also `defaultConversation`
   is a base58 UUIDv7 string on the wire (it was `z.number()`).

3. Thread.pinned: the wire shape only includes `pinned_ts` (epoch ms or
   null); the legacy `pinned` bool that the change-doc claimed would
   stick around for Zapier compatibility isn't actually present.
   Loosened to `z.boolean().optional()` — callers should derive from
   `pinnedTs != null` if they need a bool.

Also removed the unused `WORKSPACE_PLANS` import from entities.ts (was
flagged by oxlint).

Verified by re-running the smoke test against staging: getSessionUser,
getWorkspaces, getWorkspace, getWorkspaceUsers, getChannels, getInbox,
getThreads, getGroups, getConversations, createChannel +
archiveChannel + deleteChannel all return 2xx and parse cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@amix amix merged commit 7b67fb7 into amix/ft.bootstrap-comms-sdk May 20, 2026
4 checks passed
@scottlovegrove scottlovegrove deleted the amix/remove-legacy-references branch May 21, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

👀 Show PR PR must be reviewed before or after merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants