Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates most CLI commands and the TUI fetch layer from the legacy hand-rolled client to the HEY SDK, adding shared SDK initialization (auth bridge, stats hooks, error conversion) and helper utilities to keep output consistent.
Changes:
- Add SDK client bootstrap in
internal/cmd/sdk.go(auth bridge,--statshooks, error mapping, timestamp/date formatting, personal calendar recordings helpers). - Migrate multiple CLI commands (boxes/box, calendars/recordings, journal, todo, timetrack, drafts, compose, reply, habit) to use
sdk.<Service>().<Method>(ctx, ...). - Migrate TUI data fetching to the SDK with converter functions from generated SDK types → existing
internal/modelstypes, retaining the legacy client only for topic entry body gap.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/tui_test.go | Updates test helper to new TUI model constructor signature. |
| internal/tui/tui.go | Switches TUI fetches to SDK, adds SDK→model converters, keeps legacy client for topic entry bodies. |
| internal/cmd/tui.go | Runs TUI with both SDK and legacy client instances. |
| internal/cmd/todo.go | Migrates todo list/add/complete/uncomplete/delete to SDK + typed request bodies. |
| internal/cmd/timetrack.go | Migrates timetrack start/stop/current/list to SDK and unified timestamp formatting. |
| internal/cmd/sdk_test.go | Adds unit tests for UTC-safe timestamp/date formatting helpers. |
| internal/cmd/sdk.go | Introduces shared SDK initialization, auth bridge, stats hooks, error conversion, and helper utilities. |
| internal/cmd/root.go | Initializes SDK during root command setup and aggregates stats across SDK + legacy client. |
| internal/cmd/reply.go | Migrates reply command to SDK (fetch entries + create reply). |
| internal/cmd/recordings.go | Migrates calendar recordings command to SDK with typed params and formatting. |
| internal/cmd/journal_test.go | Updates journal write tests for SDK field names and adds journal read coverage (200 vs 204). |
| internal/cmd/journal.go | Migrates journal list/read/write to SDK with fallback to legacy scrape when SDK returns nil/204. |
| internal/cmd/habit.go | Migrates habit complete/uncomplete to SDK with ID parsing and normalized output. |
| internal/cmd/drafts.go | Migrates drafts listing to SDK and adjusts displayed fields. |
| internal/cmd/compose.go | Migrates compose (new message / topic message) to SDK with typed payloads. |
| internal/cmd/calendars.go | Migrates calendars list to SDK and unwraps generated calendar payload. |
| internal/cmd/boxes.go | Migrates boxes list to SDK. |
| internal/cmd/box.go | Migrates box show + postings display to SDK and adds topic-id resolution helper. |
| go.sum | Adds sums for new SDK/transitive dependencies. |
| go.mod | Adds hey-sdk module dependency (currently with local replace + placeholder version). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fa8513c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cmd/box.go">
<violation number="1" location="internal/cmd/box.go:122">
P1: Dereferencing `*result` without a nil check risks a panic if the SDK returns a nil pointer with no error (e.g., empty collection). Add a guard before the range.</violation>
</file>
<file name="go.mod">
<violation number="1" location="go.mod:18">
P0: Local-path `replace` directive will break the build for everyone except the author. This must be replaced with a real published module version (or at minimum a commit-pinned pseudo-version) before merging.</violation>
</file>
<file name="internal/tui/tui.go">
<violation number="1" location="internal/tui/tui.go:530">
P1: Nil pointer dereference if the SDK returns a nil pointer without an error. Guard with a nil check before dereferencing `*result`.</violation>
<violation number="2" location="internal/tui/tui.go:622">
P1: Nil pointer dereference if `resp` is nil. Add a nil guard before ranging over `*resp`.</violation>
<violation number="3" location="internal/tui/tui.go:665">
P1: Nil pointer dereference if `resp` is nil. Add a nil guard before ranging over `*resp`.</violation>
</file>
<file name="internal/cmd/boxes.go">
<violation number="1" location="internal/cmd/boxes.go:48">
P1: Dereferencing `result` without a nil guard risks a panic if the SDK returns `(nil, nil)`. Add a nil check or default to an empty slice.</violation>
</file>
<file name="internal/cmd/drafts.go">
<violation number="1" location="internal/cmd/drafts.go:48">
P1: Dereferencing `result` without a nil guard will panic if the SDK returns `(nil, nil)`. Guard against this before dereferencing.</violation>
</file>
<file name="internal/cmd/sdk.go">
<violation number="1" location="internal/cmd/sdk.go:130">
P2: URL parsing after `/topics/` captures trailing slashes, query strings, or sub-paths, causing `ParseInt` to fail and silently returning `p.Id` (posting ID) instead of the actual topic ID. Split on `/` or `?` to isolate the numeric segment.</violation>
</file>
<file name="internal/cmd/recordings.go">
<violation number="1" location="internal/cmd/recordings.go:80">
P1: Nil pointer dereference: `resp` is not checked for nil before dereferencing with `*resp`. If the SDK returns `(nil, nil)` — which `filterRecordingsByType` in `sdk.go` already guards against — this will panic at runtime.</violation>
</file>
<file name="internal/cmd/compose.go">
<violation number="1" location="internal/cmd/compose.go:93">
P2: Use a nil slice (`var to []string`) instead of an empty slice literal (`[]string{}`). The empty non-nil slice will serialize as `"to":[]` in JSON, changing the wire format from the old behavior which omitted the field entirely.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cmd/compose.go">
<violation number="1" location="internal/cmd/compose.go:93">
P2: `To` field lacks `omitempty` in the generated struct, so a nil slice serializes as `"to": null` instead of the previous `"to": []`. If the API rejects `null` for this field, composing without `--to` will break.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cmd/sdk.go">
<violation number="1" location="internal/cmd/sdk.go:92">
P1: Switching from `ErrNotFound` to `ErrAPI(404, ...)` changes the process exit code from 2 (`ExitNotFound`) to 7 (`ExitAPI`) and the JSON error `code` field from `"not_found"` to `"api"`. This is a silent behavioral regression for callers that check exit codes or parse `--json` error responses to distinguish not-found from other API errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/tui/tui.go:664
fetchJournalonly identifies the personal calendar viaCalendar.Personal. Elsewhere (seefindPersonalCalendarIDininternal/cmd/sdk.go) there’s a fallback to match by calendar name "Personal" as well. To avoid divergent behavior between CLI and TUI (and reduce risk of “no personal calendar found”), consider reusing the same helper logic here (or adding the name fallback).
var personalID int64
for _, cw := range payload.Calendars {
if cw.Calendar.Personal {
personalID = cw.Calendar.Id
break
}
}
if personalID == 0 {
return errMsg{fmt.Errorf("no personal calendar found")}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 18 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="API-COVERAGE.md">
<violation number="1" location="API-COVERAGE.md:17">
P2: Three CLI commands dropped from the coverage table: `hey journal list`, `hey todo list`, and `hey timetrack list` all use the `/calendars/{id}/recordings.json` endpoint via `listPersonalRecordings` but aren't mentioned. Since the purpose of this file is to track CLI-to-API mapping, they should be listed (e.g. as additional rows sharing the same endpoint, or noted in the existing recordings row).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Review carefully before merging. Consider a major version bump. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/tui/tui.go:672
fetchJournalonly identifies the personal calendar via thePersonalflag. Elsewhere (and insdk_test.go) the CLI explicitly falls back to a calendar named "Personal" when the flag isn't set; without that fallback the TUI will error with "no personal calendar found" in that scenario. Consider adding the same name-based fallback (and keeping the existing error if still not found).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Introduce hey-sdk/go v0.1.0 dependency with CLI-side plumbing: auth strategy adapter, error converter, timestamp/date formatters, calendar helpers, and stats hooks for --stats tracking.
Replace apiClient calls with typed SDK service methods across all commands: boxes, compose, drafts, habits, journal, recordings, reply, todos, and timetracks. Journal read falls back to legacy HTML scrape when the JSON endpoint returns 204.
Switch TUI data fetching from client.Client to hey.Client for boxes, calendars, recordings, and journal. Topic entry display and image fetching retain the legacy client for HTML-scrape entry bodies.
Delete internal/client/ files whose methods are fully superseded by the HEY SDK: boxes, calendars, habits, personal_recordings, time_tracks, todos, and their tests. Trim entries.go (keep GetTopicEntries), journal.go (keep GetJournalEntry), and client.go (keep Get, GetHTML, retry infra). Remove unused model types: BoxShowResponse, EntriesResponse, Draft, CalendarsResponse, CalendarWrapper, Todo, TimeTrack.
Replace the two-call List→Get pattern in hey box with direct SDK named getters (GetImbox, GetFeedbox, GetTrailbox, GetAsidebox, GetLaterbox, GetBubblebox), saving one API round-trip. Fall back to List+filter for numeric IDs and unrecognized names.
Rewrite AGENTS.md architecture section to describe the SDK-first pattern with legacy client kept only for gap operations. Rewrite API-COVERAGE.md to show SDK method vs legacy client per endpoint with gap annotations.
Replace the shared background context helper (cmdContext) with cmd.Context() in all CLI commands so SDK requests honor Cobra's cancellation. Add a cancellable context to the TUI model, cancelled on ctrl+c, so in-flight SDK requests stop when the user quits.
- Add hey todo/timetrack/journal list to API-COVERAGE.md recordings row - Guard against nil legacy client in TUI fetchTopic - Update legacy client comment to reflect current gap operations - Fix drafts agent_notes to reference subjects instead of kind - Add tests for calendar helpers (findPersonalCalendarID, unwrapCalendars, filterRecordingsByType)
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.
Summary
auth.Manager→ SDKAuthStrategy), error mapping, stats hooks, and calendar helpersapiClient.Method()tosdk.Service().Method(ctx, ...)using generated typesmodels.*for the view layerGetImbox,GetFeedbox, etc.) for well-known mailbox names, saving one API round-trip vs the old List→filter→Get patternWhat remains in
internal/client/Only methods needed for two SDK gaps:
GetTopicEntrieshey threads, TUI topic viewEntryhas no body/content fieldGetJournalEntryhey journal readfallback, TUIGetGetHTMLNew,RequestCount,TotalLatencyKnown gap:
replacedirectivego.modhas areplacedirective pointing at the local SDK checkout. This must be removed before merging — it's there for development against the unpublished SDK.Test plan
make checkpasses (fmt, vet, lint, tests, tidy)hey boxes,hey box imbox,hey calendars,hey todo list,hey journal read,hey drafts,hey --stats boxes--jsonoutput shape unchanged for read commandshey journal write,hey timetrack start/stop,hey todo add/complete/delete