Skip to content

fix: forward conversation and thread date filters#42

Open
jacobo-doist wants to merge 2 commits into
mainfrom
jacobo/fix-conversation-since
Open

fix: forward conversation and thread date filters#42
jacobo-doist wants to merge 2 commits into
mainfrom
jacobo/fix-conversation-since

Conversation

@jacobo-doist

@jacobo-doist jacobo-doist commented Jul 10, 2026

Copy link
Copy Markdown

Date filters declared by conversation view did not reach the API, so --since was silently ignored; --until would have been ignored as well.

The handler accepted both flags but only passed the conversation ID and limit to the SDK message request.

This change forwards --since and --until as the SDK date-filter arguments, with regression coverage for both flags.

While investigating the reported conversation problem, I had an agent audit related date-filter wiring. That found a second omission: thread view accepted --until but did not forward it. This PR fixes that case too.

One concern remains intentionally out of scope: the CLI flag names may imply different inclusive/exclusive boundaries from the SDK newerThan and olderThan arguments. This PR only wires the existing values through; a follow-up will verify API boundary behaviour before changing semantics.

@jacobo-doist jacobo-doist marked this pull request as ready for review July 10, 2026 12:35
@doistbot doistbot requested a review from henningmu July 10, 2026 12:35

@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 wires the --since and --until CLI flags through to the SDK's newerThan/olderThan arguments in both conversation view and thread view, fixing a gap where the date filters were accepted but silently ignored.

Few things worth tightening:

  • The options.since ? new Date(options.since) : undefined / options.until ? new Date(options.until) : undefined pattern is now duplicated across conversation/view.ts, thread/view.ts, and inbox.ts. A small shared helper (e.g. toDate(value?: string): Date | undefined) would centralize the conversion and make the planned boundary-semantics follow-up a one-line change instead of three.
  • The conversation test at line 878 uses an exact object literal in toHaveBeenCalledWith, which will break if getMessages gains another parameter. Switching to expect.objectContaining({...}) — as the thread tests already do — keeps the assertion focused on what it actually cares about.

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

Optional follow-up notes (2)
  • P3 src/commands/conversation/view.ts:43: The options.since ? new Date(options.since) : undefined / options.until ? new Date(options.until) : undefined pair is now duplicated across conversation/view.ts, thread/view.ts, and inbox.ts. Your PR description notes a follow-up to verify inclusive/exclusive boundary semantics — when that lands, any adjustment (e.g. adding/subtracting a day for --until) will need to be applied in three places. A small shared helper in src/lib/options.ts or src/lib/dates.ts (e.g. toDate(value?: string): Date | undefined) would centralize the conversion and make that follow-up a one-line change. Low priority given it's only two lines today.
  • P3 src/commands/conversation/conversation.test.ts:878: The toHaveBeenCalledWith assertion uses an exact object literal ({ conversationId, newerThan, olderThan, limit }). If getMessages gains an additional parameter in the future, this test breaks even though the date-filter forwarding still works. The thread tests for the same behavior use expect.objectContaining({...}), which is more resilient. Consider matching with expect.objectContaining here too, keeping the test focused on what it actually cares about.

Share FeedbackReview Logs

@jacobo-doist

Copy link
Copy Markdown
Author

Addressed: added toDate in src/lib/options.ts and replaced the repeated conversions in conversation view, thread view, and inbox. This preserves the existing conversion behavior; date-boundary semantics remain deferred.

-- Replied by Clanker 🤖

@jacobo-doist

Copy link
Copy Markdown
Author

Addressed: updated the conversation filter test to use expect.objectContaining(...), so it verifies the date-filter wiring without coupling to the complete SDK request shape.

-- Replied by Clanker 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants