Skip to content

Conversation

@Jingo88
Copy link
Contributor

@Jingo88 Jingo88 commented Oct 30, 2025

🎟️ Tracking

Related SDK PR

📔 Objective

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@claude
Copy link

claude bot commented Oct 30, 2025

Claude finished @Jingo88's task —— View job


Code Review Complete

Review Tasks:

  • Read and analyze package.json changes
  • Review package-lock.json changes
  • Examine commit history and messages
  • Review git diff for all changes
  • Assess version consistency and correctness
  • Provide comprehensive review feedback

Summary of Changes

This PR updates both @bitwarden/sdk-internal and @bitwarden/commercial-sdk-internal from version 0.2.0-main.357 to 0.2.0-main.365. The update was performed across 3 commits:

  1. c2ff601 - Updated @bitwarden/sdk-internal from 357 → 364
  2. 23fcb7f - Updated @bitwarden/commercial-sdk-internal from 357 → 364
  3. 89efee5 - Bumped both SDKs from 364 → 365

Final state: Both SDKs are now at version 0.2.0-main.365


Critical Issues

1. Version Mismatch in PR Title

  • Severity: Low (documentation only)
  • Location: PR title
  • Issue: The PR title says "Update SDK to version 364" but the actual final version is 365
  • Action: Update the PR title to reflect the correct version: [PM-27214] Update SDK to version 365

2. Incremental Commit Strategy

  • Severity: Medium (process/maintainability)
  • Location: Commit history (package.json:163-164)
  • Issue: The version update was split into 3 separate commits:
    • First updating sdk-internal to 364
    • Then updating commercial-sdk-internal to 364
    • Then bumping both to 365
  • Impact:
    • Creates 2 intermediate states (357→364, mixed 357/364) where SDK versions are inconsistent
    • Makes the git history harder to understand
    • Commits 1 & 2 leave the dependencies in a mismatched state
  • Recommendation: For future SDK updates, perform both SDK updates in a single atomic commit to maintain consistency throughout git history. If intermediate versions are needed for testing, consider using temporary branches instead of committing each step to the PR.
Detailed commit analysis
c2ff601: update sdk version
- Only updated sdk-internal to 364
- Left commercial-sdk at 357 (inconsistent state)

23fcb7f: update commercial sdk  
- Updated commercial-sdk to 364
- Now both at 364 (consistent, but why not in previous commit?)

89efee5b: increase version plus 1
- Updated both to 365
- Final consistent state

The question this raises: Why was version 364 used as an intermediate step? Was there testing done at 364 before moving to 365? If so, this should be documented in the PR description.


Suggested Improvements

1. Commit Message Quality

  • Location: All 3 commits
  • Current:
    • "update sdk version" (lowercase, vague)
    • "update commercial sdk" (lowercase, incomplete context)
    • "increase version plus 1" (unclear what is being increased)
  • Suggested format:
    chore: update SDK packages from 357 to 365
    
    - Update @bitwarden/sdk-internal to 0.2.0-main.365
    - Update @bitwarden/commercial-sdk-internal to 0.2.0-main.365
    
    Related to PM-27214
    Ref: https://github.com/bitwarden/sdk-internal/pull/521
    
  • Benefit: Clear, professional commit messages that explain what changed and why, following conventional commit style seen in the repo history

2. PR Description Lacks Details

  • Location: PR description
  • Issue: The PR body has empty "Objective" and "Screenshots" sections
  • Suggested additions:
    • Objective section: Explain what changes/fixes are included in SDK version 365
    • Add a note about whether this is a breaking change
    • Document any migration steps needed (if applicable)
    • Explain why version 364 was used as an intermediate step (if there's a reason)

3. Changelog/Release Notes

  • Location: Not visible in PR
  • Question: Are there release notes or a changelog in the related SDK PR that should be referenced here?
  • Suggestion: If SDK version 365 includes significant changes, consider adding a brief summary in the PR description to help reviewers understand the impact

Good Practices Observed

  • Both SDK versions kept in sync (final state)
  • package-lock.json updated consistently with package.json
  • CI checks passing (Codecov, Checkmarx)
  • Related SDK PR linked in description

Questions for Author

  1. Why the incremental versioning? Was there a specific reason for updating to 364 first, then 365? If this was for testing purposes, please document this in the PR description.

  2. Breaking changes? Does SDK version 365 introduce any breaking changes that might affect the clients codebase?

  3. Testing performed? Have you tested this update locally across different clients (web, desktop, browser, CLI)?

  4. Version 364 vs 365: The title mentions 364, but the final version is 365. Which version was originally intended?


Action Items

For Author:

  • Update PR title from "version 364" to "version 365"
  • Fill in the "Objective" section in PR description with details about what changed in SDK 365
  • (Optional but recommended) Explain the rationale for the 3-commit approach if there was a specific reason
  • (Optional) Consider squashing the 3 commits into 1 atomic commit for cleaner history

For Future SDK Updates:

  • Consider updating both SDK packages in a single commit to avoid intermediate inconsistent states
  • Use more descriptive commit messages following conventional commit format
  • Include SDK changelog highlights in the PR description

Security & Performance Assessment

  • No security vulnerabilities detected (Checkmarx passed)
  • No encryption-related code modified (package updates only)
  • No performance concerns for dependency version bumps
  • Dependency integrity hashes updated correctly in package-lock.json

Overall Assessment

This is a straightforward dependency update with no code changes beyond package versions. The main concerns are:

  1. Process-related (incremental commits creating inconsistent states)
  2. Documentation-related (title mismatch, empty PR sections)

The changes themselves are safe and follow the standard npm update pattern. Once the title is corrected and the PR description is enhanced, this is ready to merge.

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.40%. Comparing base (1e5c0ac) to head (89efee5).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17130      +/-   ##
==========================================
- Coverage   40.45%   40.40%   -0.05%     
==========================================
  Files        3503     3504       +1     
  Lines      100022   100148     +126     
  Branches    15000    15022      +22     
==========================================
+ Hits        40461    40466       +5     
- Misses      57844    57964     +120     
- Partials     1717     1718       +1     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

Logo
Checkmarx One – Scan Summary & Detailsfa2fa410-c191-4ee0-89f4-4f0b688e7bcd

Great job! No new security vulnerabilities introduced in this pull request

gbubemismith
gbubemismith previously approved these changes Oct 30, 2025
@Jingo88 Jingo88 changed the title [PM-27214] Update SDK to version 364 [PM-27214] Update SDK to version 365 Oct 30, 2025
@Jingo88 Jingo88 merged commit 98849a5 into main Oct 31, 2025
121 checks passed
@Jingo88 Jingo88 deleted the PM-27214-client-sdk-update branch October 31, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants