Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Updates the names.Resolver “me” person resolution to use the account-scoped /my/profile.json endpoint (via the SDK) rather than paginating through /people.json, improving scalability for large accounts.
Changes:
- Switch
ResolvePerson("me")to callPeople().Me()(single request) and remove email-based matching against the full people list. - Add an in-memory session cache (
r.me) for the “me” profile and clear it on cache reset / account switch. - Simplify/replace “me” resolution tests to use an injectable
resolveMeFntest hook.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/names/resolver.go | Implements /my/profile.json-based “me” resolution with session caching and cache invalidation on account changes. |
| internal/names/resolver_test.go | Updates “me” resolution tests to align with the new resolution path (using a test override). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ResolvePerson("me") was fetching the entire /people.json list (paginated)
just to match the current user's email. This doesn't scale for accounts
with thousands of users.
Now calls People().Me() — a single request returning the authenticated
user's account-scoped person record directly. This eliminates the full
people list fetch, the email string matching, and the auth.GetUserEmail()
dependency for "me" resolution.
The result is cached in-memory (r.me) for repeated use within a session,
cleared on account switch.
`people show me` was resolving "me" via People.Me() to get the person ID, then re-fetching the same person via People.Get(id). Now detects "me" directly and returns the People.Me() result without a second request.
jeremy
changed the title
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Without an explicit project, these filters silently inherited a config default and returned incomplete results scoped to one project. Now they error with a hint pointing to `basecamp reports assigned/overdue` for cross-project queries. Same fix for `todos sweep` — mutation commands should never silently inherit an ambient project.
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
"me" resolution —
ResolvePerson("me")was fetching the entire/people.jsonlist to match the current user's email. Now calls/my/profile.jsondirectly (single request), cached in-memory for the session, cleared on account switch.people show mealso short-circuits to avoid a redundant re-fetch.todos --assignee / --overdue — these filters silently inherited a config-default project and returned incomplete results scoped to one project. Now they require
--in <project>and error with a hint pointing tobasecamp reports assigned/overduefor cross-project queries. Same fix fortodos sweep.Test plan
make checkpasses (fmt, vet, lint, test, e2e)basecamp people show me -vv— singlePeople.Merequestbasecamp todos --assignee me— errors with hint to use--inorreports assignedbasecamp todos --assignee me --in <project>— works as beforebasecamp todos sweep --overdue --complete— errors requiring--in