Skip to content

[PM-36403] Group view models ts strict#21825

Draft
BTreston wants to merge 3 commits into
mainfrom
ac/pm-36403-group-view-models-ts-strict
Draft

[PM-36403] Group view models ts strict#21825
BTreston wants to merge 3 commits into
mainfrom
ac/pm-36403-group-view-models-ts-strict

Conversation

@BTreston

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/issues?filter=10675&selectedIssue=PM-36403

📔 Objective

Refactor group view, request, response models to be ts-strict

📸 Screenshots

@BTreston BTreston added t:tech-debt Change Type - Tech debt ai-review Request a Claude code review labels Jul 13, 2026
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the ts-strict refactor of the group models (GroupView, GroupDetailsView, GroupRequest) and their call sites in the group API service, plus supporting test/story updates. The change removes @ts-strict-ignore, marks fields readonly, and replaces Object.assign/field-assignment construction with explicit constructors. Verified all five construction sites were updated and that fromResponse maps the same public fields the previous Object.assign copied, making the refactor behavior-preserving.

Code Review Details

No blocking findings.

Notes considered and cleared during review:

  • Widening externalId from string to string | undefined is a compile-time type change enforced by CI (npm run test:types); no production consumer reads this field, and the ?? undefined normalization only converts a runtime null into undefined. Not a runtime break.
  • Removing Object.assign(new GroupView(), response) no longer copies BaseResponse's private response field onto views, but that field is private and unused by consumers, so behavior is unchanged.
  • The edit-member-dialog.component.stories.ts mock additions (BillingConstraintService, OrganizationMetadataServiceAbstraction) repair a Storybook broken by prior commit ce27f4b4e0; unrelated to the model refactor but a reasonable co-fix.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.35%. Comparing base (b88e564) to head (d790a7e).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ole/organizations/core/views/group-details.view.ts 0.00% 8 Missing ⚠️
...ber-dialog/edit-member-dialog.component.stories.ts 0.00% 7 Missing ⚠️
...min-console/organizations/core/views/group.view.ts 0.00% 6 Missing ⚠️
...ions/core/services/group/requests/group.request.ts 0.00% 3 Missing ⚠️
...nizations/core/services/group/group-api.service.ts 0.00% 2 Missing ⚠️
...ault/components/vault-items/vault-items.stories.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #21825      +/-   ##
==========================================
- Coverage   50.35%   50.35%   -0.01%     
==========================================
  Files        4233     4233              
  Lines      133031   133041      +10     
  Branches    20293    20300       +7     
==========================================
+ Hits        66989    66990       +1     
- Misses      61099    61109      +10     
+ Partials     4943     4942       -1     

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant