Fix resolution layer and cleanup issues - #131
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughVersion 0.7.1 release addressing seven linked issues: case-insensitive entity resolution across all types; OAuth concurrent refresh recovery and cleaner error messages; file upload/download redirect host validation; cycle delete messaging; autopagination defaults; GraphQL variable whitespace preservation; and parallel issue filter resolution. Changeslinearctl v0.7.1 Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/commands/file.test.ts (1)
381-414: ⚡ Quick winAdd a boundary test for “exactly 5 redirects then success.”
The new redirect tests are good, but they don’t assert the max-redirect boundary. A same-host chain of 5 redirects followed by
200would prevent regressions on the intended limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/file.test.ts` around lines 381 - 414, Add a new test case in the same file that exercises handleFileCommand for a same-host chain of exactly five 302 redirects followed by a 200 response: mock fetchImpl to sequentially return five 302 responses with Location headers on the same host (e.g., "https://uploads.linear.app/stepN") and then a final 200 Response, call handleFileCommand(["download", "https://uploads.linear.app/some-file.png"], { ...baseOptions(paths), fetchImpl, output: outputPath }), and assert a success exit code (0), that fetchImpl was called six times, that each request init used redirect: "manual" and preserved the authorization header ("lin_api_work") on redirects; keep the same captureOutput setup and cleanup as existing tests and mirror expectations (fetch call count, headers, redirect handling) to verify the max-redirect boundary is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/file.ts`:
- Around line 86-109: The redirect loop is off-by-one and currently prevents
following the documented 5 redirects; update the loop condition so it allows up
to 5 redirects before failing (e.g., change for (let redirectCount = 0;
redirectCount < 5; redirectCount++) to for (let redirectCount = 0; redirectCount
<= 5; redirectCount++) or otherwise use a MAX_REDIRECTS constant and compare
with <=), keeping the existing logic around fetchImpl(currentUrl,..),
isRedirectStatus(response.status), resolveRedirectUrl(...), and currentUrl
assignment intact so a final non-redirect response can be fetched after the
allowed 5 redirects.
In `@src/core/auth/runtime.ts`:
- Around line 128-172: In recoverConcurrentOAuthRefresh wrap the
refreshAccessToken(...) invocation in a try/catch: call refreshAccessToken
inside try, and on catching an OAuthTokenError (or the specific token error type
thrown by refreshAccessToken) return undefined to mirror the original
invalid_grant fallback; for other errors rethrow or convert to
ProfileResolutionError if you prefer consistent error types. Ensure the catch
prevents building refreshedCredentials, updating the store
(setCredentialsProfile/writeCredentialsFile) or returning a mutated profile when
the refresh itself failed.
---
Nitpick comments:
In `@tests/commands/file.test.ts`:
- Around line 381-414: Add a new test case in the same file that exercises
handleFileCommand for a same-host chain of exactly five 302 redirects followed
by a 200 response: mock fetchImpl to sequentially return five 302 responses with
Location headers on the same host (e.g., "https://uploads.linear.app/stepN") and
then a final 200 Response, call handleFileCommand(["download",
"https://uploads.linear.app/some-file.png"], { ...baseOptions(paths), fetchImpl,
output: outputPath }), and assert a success exit code (0), that fetchImpl was
called six times, that each request init used redirect: "manual" and preserved
the authorization header ("lin_api_work") on redirects; keep the same
captureOutput setup and cleanup as existing tests and mirror expectations (fetch
call count, headers, redirect handling) to verify the max-redirect boundary is
honored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01d512eb-ebd0-4cfb-be5e-9c07879dafb8
⛔ Files ignored due to path filters (1)
src/generated/embedded-skills.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
CHANGELOG.mddocs/commands.mdpackage.jsonskills/linearctl/SKILL.mdsrc/commands/cycle.tssrc/commands/file.tssrc/commands/gql.tssrc/commands/issue.tssrc/core/auth/oauth.tssrc/core/auth/runtime.tssrc/core/pagination/pagination.tssrc/core/resolution/resolve.tstests/commands/cycle.test.tstests/commands/file.test.tstests/commands/gql.test.tstests/commands/issue.test.tstests/commands/project.test.tstests/core/auth/oauth.test.tstests/core/auth/runtime.test.tstests/core/pagination/pagination.test.tstests/core/resolution/resolve.test.ts
Summary
Fixes #116
Fixes #117
Fixes #121
Fixes #122
Fixes #123
Fixes #125
Fixes #126
issue list --statenow uses workflow state resolution when a team scope is available, and independent issue-list resolutions run concurrently.containsIgnoreCaseproject filtering before exact/prefix/substring disambiguation. The optional disk cache from Performance: resolveProjectId downloads the entire workspace project list (twice on scoped miss); resolutions run sequentially; no caching #117 is intentionally skipped because the server-side filter removes the main performance issue without adding cache invalidation complexity.cycle deleteas an archive alias, including dry-run and JSON output.gql --varvalues and defaults bounded autopagination to max page size when no--page-sizeis set.linearctlskill.Breaking changes
None.
Verification
bun run typecheckbun run testbun run buildbun run build:binarySummary by CodeRabbit
Changed
--max/--allrequestscycle deleteis presented as an alias of archive in outputsBug Fixes
--varpreserves whitespaceDocumentation