Skip to content

fix: Accept inbox thread URLs#30

Merged
amix merged 1 commit into
mainfrom
amix/fix-inbox-thread-urls
Jun 16, 2026
Merged

fix: Accept inbox thread URLs#30
amix merged 1 commit into
mainfrom
amix/fix-inbox-thread-urls

Conversation

@amix

@amix amix commented Jun 16, 2026

Copy link
Copy Markdown
Member

Context

tdc thread view rejected SDK-generated inbox thread links such as https://comms.todoist.com/{workspace}/inbox/t/{thread}/.

What was changed

  • Parse explicit inbox/t and saved/t thread URL routes in parseCommsUrl.
  • Keep inbox roots and malformed account URLs unclassified.
  • Parameterize URL parser, resolver, and classifier regression tests.

@amix amix requested a review from scottlovegrove June 16, 2026 07:59
@amix amix marked this pull request as ready for review June 16, 2026 08:00
@amix amix added the 👀 Show PR PR must be reviewed before or after merging label Jun 16, 2026
@amix amix merged commit af8573e into main Jun 16, 2026
7 checks passed
@amix amix deleted the amix/fix-inbox-thread-urls branch June 16, 2026 08:00
doist-release-bot Bot added a commit that referenced this pull request Jun 16, 2026
## [1.6.4](v1.6.3...v1.6.4) (2026-06-16)

### Bug Fixes

* accept inbox thread URLs ([#30](#30)) ([af8573e](af8573e))
@doist-release-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.6.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR extends parseCommsUrl to support inbox and saved thread URLs, cleans up the route-pair parsing loop, and adds parameterized regression tests.

Few things worth tightening:

  • Make the inbox/t and saved/t branches stricter so they only match a t/{thread} segment with an optional c/{comment} suffix; extra trailing segments should remain unclassified to prevent malformed URLs from being misrouted as conversations.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (2)
  • [P3] src/lib/refs.ts:138: The repeated condition makes the intentional fall-through for paths like inbox/done easy to miss. Grouping by the route prefix makes the distinct cases clearer and removes the duplicate array access: typescript const route = segments[routeStart] if (route === 'inbox' || route === 'saved') { if (segments[routeStart + 1] === 't') { parseRoutePairs(routeStart + 1) } } else { parseRoutePairs(routeStart) }
  • [P3] src/lib/refs.test.ts:291: Add saved thread URL and saved thread URL with comment suffix cases to match the inbox coverage and the explicitly added saved/t route support.

Share FeedbackReview Logs

Comment thread src/lib/refs.ts
(segments[routeStart] === 'inbox' || segments[routeStart] === 'saved') &&
segments[routeStart + 1] === 't'
) {
parseRoutePairs(routeStart + 1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 P2 This reuses the generic pair parser for inbox/t and saved/t, so malformed URLs with extra route pairs are now accepted. For example, classifyCommsUrl('https://comms.todoist.com/20/inbox/t/TH1/msg/CV1') will return conversation and tdc view will route it, even though this isn't a valid conversation URL. Please parse these routes explicitly as t/{thread} with only an optional c/{comment} suffix, and leave any other trailing segments unclassified.

@scottlovegrove

Copy link
Copy Markdown
Collaborator

👍🏻

amix added a commit that referenced this pull request Jun 17, 2026
Only classify `t/{thread}` with an optional `c/{comment}` suffix after
inbox/saved; extra trailing segments stay unclassified so malformed URLs
aren't misrouted (e.g. as a conversation). Groups the branch by route
prefix and adds saved-thread + strictness test coverage.

Addresses review on #30.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released 👀 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