refactor: remove legacy Twist/migration framing#1
Merged
Conversation
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>
doistbot
reviewed
May 20, 2026
Member
doistbot
left a comment
There was a problem hiding this comment.
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.
scottlovegrove
approved these changes
May 20, 2026
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>
Member
Author
|
All 12 review comments are addressed in 480fd0f. Summary:
104 → 115 tests, type-check + lint + format all green. |
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This SDK was bootstrapped from
twist-sdk-typescript; the bootstrap commitcarried over a lot of legacy framing ("as of the UUIDv7 migration", "what's
gone vs. legacy Twist",
Comms_API_changes.mdreferences, Twist branding inthe 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
docusaurus.config.ts,docs/index.md,docs/authorization.md): renamedTwist*→Comms*everywhere, switched logo/social-card to the already-present Todoist assets, removedtwist-logo.png.Comms_API_changes.mdreferences, and "as of the Todoist-id migration" prose from every client docstring and fromsrc/types/{entities,requests,enums}.ts,src/utils/{uuidv7,url-helpers}.ts, and test-default comments.StatusOkSchema/StatusOkhoisted intosrc/types/entities.ts, per-clientXListSchema = z.array(XSchema)helpers, andadd-comment-helper.tscombines bothgroups/directGroupMentionsmarker offenses into a single error.Out of scope
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.tsstill uses inlinez.array(InboxThreadSchema)rather than a hoistedInboxThreadListSchema; mirroring the rest of the codebase can be a follow-up.