feat: add the issue commands - #9
Open
paulo-granthon wants to merge 33 commits into
Open
Conversation
Adds the `issue` context: open, list, view, comment, close and reopen.
:AgitateIssueCreate
:AgitateIssueList -s all
:AgitateIssueView -n 42
:AgitateIssueComment -n 42
:AgitateIssueClose -n 42
:AgitateIssueReopen -n 42
Every command resolves the repository from the `origin` remote, so the
common case takes no arguments. `-u` and `-r` override it.
The list is interactive: `<CR>` views, `o` opens in the browser, `c`
comments, `x` closes, `q` quits. The same operations are also plain
commands, so they work from a keybinding without going through the
list first.
Two details worth calling out.
GitHub's issues endpoint also returns pull requests. That is a long
standing quirk of the API rather than something the caller asked for,
and an issue list showing pull requests would simply be wrong. Entries
carrying a `pull_request` key are filtered out in the service, with a
test covering it.
Fetching an issue and fetching its comments are separate requests. If
the comments fail, the issue is still shown and the failure is reported
as a warning, rather than throwing away a view the user can use.
Closing does not ask for confirmation. It is reversible, and
`:AgitateIssueReopen` is right there.
Eleven service tests: the pull request filtering, the requested state
reaching the query, a failure passing through unfiltered, and the
create, state and comment calls hitting the right method, URL and body.
There was a problem hiding this comment.
🟡 Changes recommended
A few concrete robustness issues (issue-number validation, pagination limits, and missing response-field validation) should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds first-class Issue support to Agitate, wiring new user commands into the existing GitHub service layer and UI buffers to create/list/view/comment/close/reopen issues with sensible defaults from the origin remote.
Changes:
- Add issue orchestration in
core.issueand register new:AgitateIssue*commands. - Extend the GitHub service with issue endpoints (list/get/create/state/comments) and add service-level tests.
- Document the new Issue context and commands in the README.
File summaries
| File | Description |
|---|---|
| tests/github_spec.lua | Adds service-level tests for issue listing/filtering and issue mutations. |
| README.md | Documents the new Issue context and updates the planned-features checklist. |
| lua/agitate/service/github.lua | Adds GitHub REST endpoints for issues and comments, plus issue list filtering. |
| lua/agitate/core/issue.lua | Implements command orchestration and UI flows for issue operations. |
| lua/agitate/api/issue.lua | Registers the new :AgitateIssue* user commands. |
| lua/agitate/api/init.lua | Hooks issue command registration into the plugin setup. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+78
to
+86
| local number = tonumber(value) | ||
|
|
||
| if not number then | ||
| agitate_error.throw(command .. ' -- Error: expected an issue number, got `' .. tostring(value) .. '`') | ||
|
|
||
| return nil | ||
| end | ||
|
|
||
| return number |
Comment on lines
+192
to
+213
| function M.list_issues(access_token, owner, repository, state, callback) | ||
| M.get( | ||
| access_token, | ||
| 'repos/' .. owner .. '/' .. repository .. '/issues?state=' .. state .. '&per_page=100', | ||
| 'list the issues of `' .. owner .. '/' .. repository .. '`', | ||
| function(list_ok, result) | ||
| if not list_ok then | ||
| return callback(false, result) | ||
| end | ||
|
|
||
| local issues = {} | ||
|
|
||
| for _, entry in ipairs(result) do | ||
| if not entry.pull_request then | ||
| issues[#issues + 1] = entry | ||
| end | ||
| end | ||
|
|
||
| callback(true, issues) | ||
| end | ||
| ) | ||
| end |
Comment on lines
+231
to
+233
| function M.list_comments(access_token, owner, repository, number, callback) | ||
| M.get(access_token, 'repos/' .. owner .. '/' .. repository .. '/issues/' .. number .. '/comments?per_page=100', 'read the comments on #' .. number, callback) | ||
| end |
Comment on lines
+241
to
+247
| function M.create_issue(access_token, owner, repository, issue, callback) | ||
| M.call(access_token, { | ||
| path = 'repos/' .. owner .. '/' .. repository .. '/issues', | ||
| method = 'POST', | ||
| body = { title = issue.title, body = issue.body }, | ||
| }, 'open an issue on `' .. owner .. '/' .. repository .. '`', 201, callback) | ||
| end |
The merge and the review fixes landed together because the conflict resolution and one of the fixes are the same change. From the base: the shared `list_comments` and the paging helper. `service.github` ended up with two `list_comments` definitions, one from each side; this branch's copy is dropped, because the base version is the one both features share and it pages instead of truncating at 100. `tests/github_spec.lua` conflicted with both sides appending describe blocks. Rebuilt from the two parents block by block rather than by hand, after a first attempt spliced two blocks into each other and left the file unparseable. Review findings addressed here: `list_issues` requested `per_page=100` and read a single page, so a repository with more than 100 issues silently showed the first hundred as if that were all of them. It now goes through `get_all`. `create_issue` returned the decoded body unchecked, and the caller announces the new issue by concatenating `number` and `html_url`, so a success missing either would have crashed while reporting itself. Both are validated, matching what `create_repository` already did. `issue_number` accepted anything `tonumber` accepted, so `3.14`, `0` and `-1` all passed and went on to build a path GitHub cannot answer. It now requires a positive integer and says so.
There was a problem hiding this comment.
🟡 Changes recommended
The new issue comment UI help text is inaccurate relative to the shared buffer parsing behavior and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+198
to
+201
| help = { | ||
| 'Write your comment. The first line is not treated specially here.', | ||
| 'Write the buffer to submit, close it without writing to abandon.', | ||
| }, |
The comment buffer told the user the first line was not treated specially, while the buffer went through `parse`, which trims the first line and collapses the blank line after it. Both statements could not be true, and the one the user reads was the wrong one: leading whitespace on the first line disappeared without explanation. Uses the `raw` mode added to `ui.buffer`, so the comment is submitted exactly as written and the help text is now accurate.
The same defect the review raised against the pull request list. The browser action passed `entry.html_url` straight to `vim.ui.open`, so an entry without one produced an error from inside the UI layer rather than a message naming the issue. Fixed here too rather than waiting for it to be reported separately, since it is the same line in the sibling command.
There was a problem hiding this comment.
🟡 Changes recommended
core.issue.prepare() drops parse_args’s incomplete return value, so missing-flag-value invocations aren’t reported correctly and can lead to unintended fallbacks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+58
to
+62
| local parameters, leftover = parse_args(flags, optional_parameters) | ||
|
|
||
| if #leftover > 0 then | ||
| return agitate_error.throw(command .. ' -- Error: unrecognised arguments: ' .. table.concat(leftover, ' ')) | ||
| end |
`prepare` captured only the first two return values from `parse_args`, so every issue command ignored `incomplete`. `:AgitateIssueView -n` reported nothing and fell back to the defaults, which means acting on a different issue than the one being asked for rather than refusing. Reports the flag that needs a value, as the repository, branch and file commands already do. Fixing it in `prepare` covers all six issue commands at once.
There was a problem hiding this comment.
🟡 Changes recommended
Two new modules return from failed require paths instead of raising, which can make pcall(require, ...) succeed with a nil module and then crash callers at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
lua/agitate/core/issue.lua:11
- On
agitate.utilimport failure the module callsagitate_error.throw(...)and then returns, which means therequirecan still succeed (returningnil) and downstream code will fail with a confusing "attempt to index a nil value". To keep failure handling consistent with other core modules, throw + raise an error to abort module loading.
local util_ok, util_or_err = pcall(require, 'agitate.util')
if not util_ok then
return agitate_error.throw(util_or_err)
end
lua/agitate/core/issue.lua:16
- Same pattern as the util import: if
agitate.service.githubfails to load,throw+returnallowsrequire('agitate.core.issue')to succeed withnil. Raise after notifying so the requiringpcallsees the failure and callers don't crash later when indexing the module.
local github_ok, github_or_err = pcall(require, 'agitate.service.github')
if not github_ok then
return agitate_error.throw(github_or_err)
end
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
Both issue modules still carried the ineffective load guard. They were created on this branch, so the sweep that fixed the rest of the plugin never reached them, and they had the same defect: `require` turns a nil module return into `true`, so `pcall(require, 'agitate.core.issue')` succeeded and `issue_or_err[method](...)` crashed while indexing a boolean. Both notify and then raise, so the guard in `api/init.lua` reports the real failure.
There was a problem hiding this comment.
🟢 Approval recommended
The feature is implemented coherently with good service-layer test coverage, and the remaining feedback is limited to small UX/API-surface refinements rather than correctness issues.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+119
to
+121
| if not comments_ok then | ||
| vim.notify('Could not load the comments for #' .. number .. '.', vim.log.levels.WARN) | ||
| end |
Comment on lines
+206
to
+210
| ---Adds a comment to an issue, given an already resolved context. | ||
| ---@param resolved table | ||
| ---@param number number | ||
| function M.comment(resolved, number) | ||
| editor.open({ |
… comments failed `comment` and `close` were exported in lower case, which put them alongside the PascalCase commands and read as if the module had a second, differently styled public API. They exist so the list keymaps and the commands can share one implementation, so they take the underscore prefix already used for tested internals in `service.http` and `core.branch`. The comment fetch failure notified that comments could not be loaded and discarded the error object it had in hand. An expired token, a rate limit and a network failure all arrive here and call for different responses, so the reason is now included. The issue itself is still shown, since a missing comment list is not a reason to withhold it.
There was a problem hiding this comment.
🔵 Needs a closer look
The new async issue view opens buffers in the currently active window at callback time (can replace the wrong window) and should be made window-stable before merging.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lua/agitate/core/issue.lua:126
document.open()uses window 0 at callback time; since the HTTP requests are async, the issue view can end up opening in whatever window is currently active rather than the window where the user invoked the action. Capturing the originating window and temporarily switching fordocument.open()avoids surprising buffer replacement when users move between windows while the request is in flight.
github.list_comments(resolved.token, resolved.owner, resolved.repository, number, function(comments_ok, comments)
-- The issue itself is worth showing even if the comments could not be
-- fetched, so a failure here degrades rather than aborts.
document.open(
'agitate://issue/' .. resolved.owner .. '/' .. resolved.repository .. '/' .. number,
lua/agitate/core/issue.lua:100
- When
-nis omitted entirely, the current error becomes “expected a positive issue number, gotnil”, which is accurate but not very actionable. Since the commands that call this helper require an issue number, report it as a missing required flag to guide the user directly.
local function issue_number(value, command)
local number = tonumber(value)
-- Issue numbers are positive integers. `3.14`, `0` and `-1` all survive
-- `tonumber` and would go on to build a path GitHub cannot answer, so the
-- mistake is worth naming here rather than as a 404 later.
if not number or number < 1 or number % 1 ~= 0 then
agitate_error.throw(command .. ' -- Error: expected a positive issue number, got `' .. tostring(value) .. '`')
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
`:AgitateIssueList -s open` and `-s closed` produced buffers with the same name for the same repository, so the second reused or collided with the first and `:ls` could not tell them apart. That is the same collision the create and comment buffers were qualified to avoid.
There was a problem hiding this comment.
🟢 Approval recommended
The feature is fully wired end-to-end with service-level tests and only a minor usability nit was found in an error message for missing -n.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+87
to
+97
| local function issue_number(value, command) | ||
| local number = tonumber(value) | ||
|
|
||
| -- Issue numbers are positive integers. `3.14`, `0` and `-1` all survive | ||
| -- `tonumber` and would go on to build a path GitHub cannot answer, so the | ||
| -- mistake is worth naming here rather than as a 404 later. | ||
| if not number or number < 1 or number % 1 ~= 0 then | ||
| agitate_error.throw(command .. ' -- Error: expected a positive issue number, got `' .. tostring(value) .. '`') | ||
|
|
||
| return nil | ||
| end |
Omitting `-n` reported "expected a positive number, got `nil`", which is accurate and tells the user nothing about what to do. A missing flag and a malformed value are different mistakes; the first now says to pass `-n <number>`.
There was a problem hiding this comment.
🟡 Changes recommended
service.github.create_issue can crash on a valid non-table JSON success body due to missing type-guarding before indexing result.number/result.html_url.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+343
to
+349
| -- The caller reports the new issue by concatenating both of these, so a | ||
| -- success missing either would crash while announcing itself. | ||
| if not result.number or not result.html_url then | ||
| return callback(false, { | ||
| message = 'service.github.create_issue -- Error: GitHub reported success but returned no `number` or `html_url`.', | ||
| }) | ||
| end |
`create_issue` indexed `result.number` directly. The transport documents `body` as any JSON type, so a valid response decoding to a string or a boolean raised on the index instead of being reported as a malformed success. `create_repository` already guarded this; the two checks are now the same. The list's browser action moves onto `util.open_url`'s return value rather than pre-checking the field itself, so there is one definition of what counts as an openable URL. Also sweeps the remaining `table<string>` annotations in this module to `string[]`, rather than waiting for each to be reported.
Missed by the sweep because it uses the `table<string>|nil` form rather than the `? table<string>` one.
There was a problem hiding this comment.
🟡 Changes recommended
service.github.get_issue currently doesn’t validate the decoded JSON type, which can surface as a runtime crash in callers when a malformed-but-JSON response is treated as success.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+320
to
+322
| function M.get_issue(access_token, owner, repository, number, callback) | ||
| M.get(access_token, 'repos/' .. owner .. '/' .. repository .. '/issues/' .. number, 'read issue #' .. number, callback) | ||
| end |
`table<string>` is not valid LuaLS and describes the wrong shape: the value is `opts.fargs`, a `string[]`. And the notification split "Repository" across a conditional (`'Private r' or 'R'` followed by `'epository '`). Both have now been reported on several branches one file at a time, because each branch holds its own copy of the affected function. Applied across every file on every branch at once so they stop recurring.
`get_issue` passed the decoded body straight through. A 200 guarantees valid JSON, not an object, so a proxy or outage page decoding to a string or boolean was reported as success and then failed in the renderer, which indexes `title`, `state` and `user`. Failing at the service boundary names the endpoint instead.
There was a problem hiding this comment.
🟢 Approval recommended
The feature is cohesive, aligns with the PR description (including PR filtering and comment-fetch degradation), and the only noted issue is a minor test-fixture consistency nit.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| it('filters out pull requests', function() | ||
| respond_with(true, { | ||
| status = 200, | ||
| raw = '[]', |
…stent The stub declared `raw = '[]'` alongside a `body` holding three entries. Only `body` is read today, and only by the filter under test, so nothing was wrong, but a fixture that contradicts itself misleads whoever reads it next. Derives `raw` from `body`.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and well-tested at the service layer, and the command/UI orchestration follows established patterns already present in the codebase.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Adds the Issue context.
Every command resolves the repository from the
originremote, so the common case takes no arguments at all.-uand-roverride it.The list is interactive
The same operations are also plain commands, so they work from a keybinding without going through the list first.
Two API details worth flagging
GitHub's issues endpoint also returns pull requests. This is a long-standing quirk rather than something the caller asks for. An issue list showing PRs mixed in would just be wrong, so entries carrying a
pull_requestkey are filtered out in the service layer. There is a test pinning it, because it is the kind of thing that looks like a bug when it regresses.Comments are a separate request from the issue. If the comment fetch fails, the issue is still displayed and the failure is reported as a warning. Throwing away a view the user can perfectly well read, because a secondary request failed, would be the wrong trade.
Closing does not confirm
Unlike branch deletion (#4) and going public (#6), closing an issue is reversible and
:AgitateIssueReopenis right there. Confirming every reversible action is how people learn to dismiss confirmations without reading them.Tests
Eleven new on the service: PR filtering, the requested state reaching the query string, a failure passing through unfiltered rather than being treated as an empty list, and the create/state/comment calls hitting the right method, URL and body.
The command layer is orchestration over pieces tested elsewhere:
parse_args(#2), the buffers and renderer (#8), and the service calls here. What is not directly tested is the wiring between them.Note on
-n-ntakes the issue number and is validated, so:AgitateIssueView -n abcreports rather than requesting/issues/nil.