fix: skip task completion when already completed#218
Merged
liangshuo-1 merged 1 commit intolarksuite:mainfrom Apr 3, 2026
Merged
fix: skip task completion when already completed#218liangshuo-1 merged 1 commit intolarksuite:mainfrom
liangshuo-1 merged 1 commit intolarksuite:mainfrom
Conversation
github-actions
bot
added
domain/task
|
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 (8)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExecute for task-complete now GETs the task first and skips PATCH if Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Shortcut as Shortcut.Execute
participant API as Task API
User->>Shortcut: +complete --task-id {id}
Shortcut->>API: GET /open-apis/task/v2/tasks/{id}
API-->>Shortcut: task data (includes completed_at)
alt completed_at present and != "0"
Shortcut-->User: return GET result (skip PATCH)
else not completed
Shortcut->>API: PATCH /open-apis/task/v2/tasks/{id} (buildCompleteBody)
API-->>Shortcut: patched task data
Shortcut-->User: return patched result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
got it !
|
Greptile SummaryThis PR makes
Confidence Score: 5/5This PR is safe to merge — both changes are correct, well-tested, and the previous review concerns have been fully addressed. No P0 or P1 issues found. The idempotency logic correctly handles all edge cases (nil task map, missing field, zero value). The local-time fix is a straightforward and correct API change. Prior concerns (sync.Once warm-up, package-level struct mutation) are resolved cleanly in the new test helpers. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (task +complete)
participant API as Lark Task API
CLI->>API: GET /open-apis/task/v2/tasks/{id}
API-->>CLI: task data (incl. completed_at)
alt completed_at != "" and != "0"
Note over CLI: Task already completed — skip PATCH
CLI->>CLI: Use GET response as output data
else task not yet completed
CLI->>API: PATCH /open-apis/task/v2/tasks/{id}
API-->>CLI: updated task data
end
CLI->>CLI: Render output (pretty / JSON)
Reviews (4): Last reviewed commit: "fix: skip task completion when already c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/task/task_complete_test.go (1)
21-50: Consider adding a test case for "already completed json format".The test covers
prettyformat for already-completed tasks andjsonformat for not-completed tasks, but there's no test case verifying JSON output when the task is already completed. This would ensure the skip-PATCH path also produces correct JSON output.➕ Proposed additional test case
{ name: "task not completed json format", taskId: "task-789", isCompleted: false, formatFlag: "json", expectedOutput: []string{ `"guid": "task-789"`, }, }, + { + name: "task already completed json format", + taskId: "task-abc", + isCompleted: true, + formatFlag: "json", + expectedOutput: []string{ + `"guid": "task-abc"`, + }, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/task/task_complete_test.go` around lines 21 - 50, Add a test case to the tests slice in task_complete_test.go that exercises the JSON output path when a task is already completed: create an entry with name like "task already completed json format", taskId (e.g., "task-123"), isCompleted: true, formatFlag: "json", and expectedOutput containing the JSON field(s) you expect (e.g., a `"guid": "task-123"` fragment). Ensure the new test struct uses the same fields as existing cases (name, taskId, isCompleted, formatFlag, expectedOutput) so it participates in the same test loop that calls the task completion handler.shortcuts/task/task_complete.go (1)
33-40: DryRun still shows PATCH even when task might be completed.The
DryRunfunction always returns a PATCH operation, which doesn't reflect the actual runtime behavior where a GET is performed first. This is likely acceptable since dry-run is meant to show the potential write operation, but users might be confused if they see "PATCH" in dry-run output for an already-completed task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/task/task_complete.go` around lines 33 - 40, The DryRun function always returns a PATCH which can mislead users when the real flow does a preliminary GET for already-completed tasks; update DryRun (in the DryRun closure) to inspect a runtime flag (e.g., runtime.Str("task-status") or a boolean like runtime.Bool("task-completed")) and if the task is already completed return a GET dry-run using the same taskId (common.NewDryRunAPI().GET("/open-apis/task/v2/tasks/"+taskId).Params(...)), otherwise keep returning the PATCH with buildCompleteBody(); refer to the DryRun function, buildCompleteBody, and common.NewDryRunAPI to implement this conditional behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/task/task_complete_test.go`:
- Around line 21-50: Add a test case to the tests slice in task_complete_test.go
that exercises the JSON output path when a task is already completed: create an
entry with name like "task already completed json format", taskId (e.g.,
"task-123"), isCompleted: true, formatFlag: "json", and expectedOutput
containing the JSON field(s) you expect (e.g., a `"guid": "task-123"` fragment).
Ensure the new test struct uses the same fields as existing cases (name, taskId,
isCompleted, formatFlag, expectedOutput) so it participates in the same test
loop that calls the task completion handler.
In `@shortcuts/task/task_complete.go`:
- Around line 33-40: The DryRun function always returns a PATCH which can
mislead users when the real flow does a preliminary GET for already-completed
tasks; update DryRun (in the DryRun closure) to inspect a runtime flag (e.g.,
runtime.Str("task-status") or a boolean like runtime.Bool("task-completed")) and
if the task is already completed return a GET dry-run using the same taskId
(common.NewDryRunAPI().GET("/open-apis/task/v2/tasks/"+taskId).Params(...)),
otherwise keep returning the PATCH with buildCompleteBody(); refer to the DryRun
function, buildCompleteBody, and common.NewDryRunAPI to implement this
conditional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 760add5d-eb84-48b4-a2a7-fd130fa12c28
📒 Files selected for processing (7)
shortcuts/task/task_complete.goshortcuts/task/task_complete_test.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_my_tasks_test.goshortcuts/task/task_shortcut_test.goskills/lark-task/SKILL.mdskills/lark-task/references/lark-task-reminder.md
ILUO
force-pushed
the
fix/task-complete-skip-if-completed
branch
from
dd113fc to
1963f46
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-task/SKILL.md`:
- Around line 24-25: The blockquote in SKILL.md contains extra blank lines that
violate markdownlint rule MD028; remove any empty lines between the lines that
start with the blockquote marker (the leading '>' characters) so the quote lines
are contiguous (ensure every quoted line begins with '>' with no intervening
blank lines), preserving the existing quote text and indentation.
🪄 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: c7dfdcc4-571f-4a37-b69d-038bef6f7d01
📒 Files selected for processing (7)
shortcuts/task/task_complete.goshortcuts/task/task_complete_test.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_my_tasks_test.goshortcuts/task/task_shortcut_test.goskills/lark-task/SKILL.mdskills/lark-task/references/lark-task-reminder.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-task/references/lark-task-reminder.md
- shortcuts/task/task_complete_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/task/task_get_my_tasks.go
- shortcuts/task/task_get_my_tasks_test.go
LuckyTerry
approved these changes
Apr 2, 2026
ILUO
force-pushed
the
fix/task-complete-skip-if-completed
branch
from
1963f46 to
27100dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
skills/lark-task/SKILL.md (1)
24-25:⚠️ Potential issue | 🟡 MinorFix blockquote blank lines to satisfy markdownlint MD028.
This issue was already identified in a previous review. There are blank lines inside the blockquote block, which triggers lint warnings. To properly separate the two blockquote sections, remove the
>marker from these blank lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-task/SKILL.md` around lines 24 - 25, The blockquote in SKILL.md contains blank lines that still include the '>' marker, triggering markdownlint MD028; edit the blockquote so blank separator lines do not start with '>' (remove the '>' from those empty lines) to properly separate the two blockquote sections and eliminate the MD028 warning while keeping the rest of the '>'-prefixed quoted lines intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/task/task_complete_test.go`:
- Around line 101-108: The current fragile normalization uses
strings.ReplaceAll(out, `":"`, `": "`) in the test, which can corrupt non-JSON
content; replace that logic by detecting and normalizing JSON output: check
json.Valid(out) (or attempt json.Unmarshal into a variable), and if it is valid
JSON unmarshal and re-marshal (e.g., json.MarshalIndent or json.Marshal) to
produce a stable normalized string (assign to outNorm), otherwise keep out
unchanged; update the checks that iterate tt.expectedOutput to use the
normalized outNorm and fall back to out when not JSON, referencing the variables
out, outNorm, and tt.expectedOutput and removing the broad strings.ReplaceAll
call.
---
Duplicate comments:
In `@skills/lark-task/SKILL.md`:
- Around line 24-25: The blockquote in SKILL.md contains blank lines that still
include the '>' marker, triggering markdownlint MD028; edit the blockquote so
blank separator lines do not start with '>' (remove the '>' from those empty
lines) to properly separate the two blockquote sections and eliminate the MD028
warning while keeping the rest of the '>'-prefixed quoted lines intact.
🪄 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: f0a0ce35-f754-42fa-9bcf-a70e433e4677
📒 Files selected for processing (8)
shortcuts/task/task_complete.goshortcuts/task/task_complete_test.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_my_tasks_test.goshortcuts/task/task_shortcut_test.goskills/lark-task/SKILL.mdskills/lark-task/references/lark-task-get-my-tasks.mdskills/lark-task/references/lark-task-reminder.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-task/references/lark-task-get-my-tasks.md
- skills/lark-task/references/lark-task-reminder.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/task/task_get_my_tasks.go
- shortcuts/task/task_complete.go
- shortcuts/task/task_get_my_tasks_test.go
ILUO
force-pushed
the
fix/task-complete-skip-if-completed
branch
from
27100dc to
1eddd92
Compare
LuckyTerry
approved these changes
Apr 3, 2026
liangshuo-1
approved these changes
Apr 3, 2026
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1eddd929c42d0104b80fecacd7caa2862fefd4fa🧩 Skill updatenpx skills add ILUO/cli#fix/task-complete-skip-if-completed -y -g |
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
4 participants
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
Avoid redundant completion requests by checking the current task status before issuing a PATCH.
This makes
task +completeidempotent for already-completed tasks.Changes
shortcuts/task/task_complete.gocreated_at/due_atusing local time inshortcuts/task/task_get_my_tasks.go+complete,+get-my-tasks)skills/lark-taskdocs: reminder prerequisites and local-time rendering notesTest Plan
go test ./shortcuts/task -run "TestCompleteTask|TestGetMyTasks_LocalTimeFormatting" -vManual verification
lark task +complete --task-id <id> --as bot --format prettylark task +get-my-tasks --as user --format prettyRelated Issues
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests