Address PR review feedback from SDK migration - #73
Conversation
Fixes for Codex review comments: 1. webhooks.go: Let server use default webhook types instead of hard-coding (PR #58 feedback) 2. templates.go: Use raw API for archived/trashed status since SDK List() only returns active templates (PR #60 feedback) 3. campfire.go: Validate ParseInt errors before DeleteLine to avoid silent 0 IDs (PR #64 feedback) 4. recordings.go: Add comment clarifying that Unarchive() works for both archived AND trashed recordings via status/active endpoint (PR #68) 5. files.go: Preserve SDK errors in auto-detect mode instead of masking auth/permission errors as "not found" (PR #70 feedback)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56438fec96
ℹ️ 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".
| // If all probes failed, check if first error was 404 or something else | ||
| if result == nil && firstErr != nil { | ||
| sdkErr := basecamp.AsError(firstErr) | ||
| if sdkErr.Code != basecamp.CodeNotFound { | ||
| // Return actual error (auth, permission, network, etc.) | ||
| return convertSDKError(firstErr) |
There was a problem hiding this comment.
When auto-detecting the item type, only the first probe's error is checked; if the vault probe returns 404 but a later upload/document probe returns 403/401/500 (e.g., item is an upload but the user lacks upload permissions), the code still falls through to ErrNotFound and masks the real failure. This defeats the stated goal of preserving auth/permission/network errors whenever a later probe is the one that actually fails.
Useful? React with 👍 / 👎.
| // All probes failed - check if first error was 404 or something else | ||
| sdkErr := basecamp.AsError(firstErr) | ||
| if sdkErr.Code != basecamp.CodeNotFound { | ||
| // Return actual error (auth, permission, network, etc.) | ||
| return convertSDKError(firstErr) |
There was a problem hiding this comment.
The update path has the same issue: only firstErr (from the document probe) is checked. If that probe returns 404 but a later vault/upload probe returns a non-404 error (permission, auth, transient API error), the command still reports "Item not found" instead of surfacing the actual failure. Users who lack access to the specific item type will get a misleading not-found response.
Useful? React with 👍 / 👎.
Fixes for Codex review comments: 1. webhooks.go: Let server use default webhook types instead of hard-coding (PR #58 feedback) 2. templates.go: Use raw API for archived/trashed status since SDK List() only returns active templates (PR #60 feedback) 3. campfire.go: Validate ParseInt errors before DeleteLine to avoid silent 0 IDs (PR #64 feedback) 4. recordings.go: Add comment clarifying that Unarchive() works for both archived AND trashed recordings via status/active endpoint (PR #68) 5. files.go: Preserve SDK errors in auto-detect mode instead of masking auth/permission errors as "not found" (PR #70 feedback)
Fixes for Codex review comments:
webhooks.go: Let server use default webhook types instead of hard-coding
(PR Migrate webhooks.go to use SDK WebhooksService #58 feedback)
templates.go: Use raw API for archived/trashed status since SDK List()
only returns active templates (PR Migrate templates.go to use SDK TemplatesService #60 feedback)
campfire.go: Validate ParseInt errors before DeleteLine to avoid
silent 0 IDs (PR Migrate campfire.go to use SDK CampfiresService #64 feedback)
recordings.go: Add comment clarifying that Unarchive() works for both
archived AND trashed recordings via status/active endpoint (PR Migrate recordings.go to use SDK RecordingsService #68)
files.go: Preserve SDK errors in auto-detect mode instead of masking
auth/permission errors as "not found" (PR Migrate files.go to use SDK VaultsService #70 feedback)