Skip to content

fix: address Base attachment review follow-ups#958

Merged
zgz2048 merged 1 commit into
larksuite:mainfrom
zgz2048:codex/base-attachment-review-fixes
May 19, 2026
Merged

fix: address Base attachment review follow-ups#958
zgz2048 merged 1 commit into
larksuite:mainfrom
zgz2048:codex/base-attachment-review-fixes

Conversation

@zgz2048
Copy link
Copy Markdown
Collaborator

@zgz2048 zgz2048 commented May 19, 2026

Summary

Address the two remaining review follow-ups from the Base attachment API work. This narrows the image-dimension warning so unsupported image MIME types like WebP do not emit a misleading message, and renames a download test to match the behavior it actually verifies.

Changes

  • Restrict the "image dimensions unavailable" warning to MIME types whose dimensions are currently decoded (image/gif, image/jpeg, image/png)
  • Add unit coverage for the warning eligibility helper, including the image/webp case
  • Rename the download execute test to reflect that it validates the outgoing extra query parameter rather than a response field

Test Plan

  • Unit tests pass
  • Manual local verification confirms the lark xxx command works as expected

Related Issues

Summary by CodeRabbit

  • Bug Fixes

    • Refined the "image dimensions unavailable" warning during attachment uploads to display only for GIF, JPEG, and PNG formats, reducing unnecessary warnings for other image types.
  • Tests

    • Added test coverage for the image dimension warning behavior.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e16cb4a7-56c5-44f5-874d-946b6275dac8

📥 Commits

Reviewing files that changed from the base of the PR and between 2bb69d1 and fbba475.

📒 Files selected for processing (3)
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/record_upload_attachment.go
  • shortcuts/base/record_upload_attachment_test.go

📝 Walkthrough

Walkthrough

This PR restricts image dimension unavailable warnings during attachment upload from all image types to GIF, JPEG, and PNG only. A new MIME-type filter helper enables this logic, integrated into the upload function, with test coverage for the helper and a clarified test name.

Changes

Image dimension warning MIME-type filter

Layer / File(s) Summary
MIME-type filter helper and integration
shortcuts/base/record_upload_attachment.go
New attachmentImageDimensionsWarningEnabled(mimeType string) bool function restricts image dimension warnings to image/gif, image/jpeg, and image/png; uploadAttachmentToBase calls this helper instead of checking a generic image/* prefix.
Test coverage and test name update
shortcuts/base/record_upload_attachment_test.go, shortcuts/base/base_execute_test.go
New table-driven test TestAttachmentImageDimensionsWarningEnabled validates the helper returns true for GIF/JPEG/PNG and false for WebP/PDF; download attachment test name clarified to reflect extra query parameter behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • larksuite/cli#887: Introduces Base attachment API codepaths that this PR's image-dimension warning updates refine.
  • larksuite/cli#563: Also modifies record_upload_attachment.go's MIME-type handling; this PR adjusts how detected MIME types control warning behavior.

Suggested reviewers

  • kongenpei
  • timzhong1024

Poem

🐰 A filter so fine, with precision and care,
GIFs, JPEGs, PNGs—the warnings we share,
WebP and PDF fade from the warning light,
Your attachment uploads, now perfectly right! 📸

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: address Base attachment review follow-ups' clearly and concisely summarizes the main changes addressing follow-up feedback on Base attachment functionality.
Description check ✅ Passed The PR description includes all required sections with complete information: a clear summary, specific changes listed, a test plan with unit tests marked as passing, and a related issues link.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added domain/base PR touches the base domain size/L Large or sensitive change across domains or core paths labels May 19, 2026
@zgz2048 zgz2048 force-pushed the codex/base-attachment-review-fixes branch from aaa507a to fbba475 Compare May 19, 2026 04:09
@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed size/L Large or sensitive change across domains or core paths labels May 19, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 66.96%. Comparing base (2bb69d1) to head (fbba475).

Files with missing lines Patch % Lines
shortcuts/base/record_upload_attachment.go 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #958   +/-   ##
=======================================
  Coverage   66.96%   66.96%           
=======================================
  Files         568      568           
  Lines       53317    53323    +6     
=======================================
+ Hits        35703    35709    +6     
  Misses      14649    14649           
  Partials     2965     2965           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zgz2048 zgz2048 marked this pull request as ready for review May 19, 2026 04:17
@github-actions
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fbba4758a82351630273ef1c9cf0762b758933a6

🧩 Skill update

npx skills add zgz2048/cli#codex/base-attachment-review-fixes -y -g

@zgz2048 zgz2048 merged commit 3354494 into larksuite:main May 19, 2026
20 checks passed
@liangshuo-1 liangshuo-1 mentioned this pull request May 19, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/base PR touches the base domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants