PM-22143 Refactor TS enums to be const objects (Send specific enums) #16399
Conversation
…nto PM-24105-Tools-Remove-getUserKey-on-the-key-service merged PM-24099-Tools-Remove-getOrgKey-from-the-key-service to avoid redundant refactor
Functional testing will be completed before review / merge for the following functionality in every applicable app: • send • individual and org encrypted export • individual import • bitwarden encrypted JSON import
Will have conflicts with PM-24099, wait for PM-24099 review before beginning to resolve
• Wait for non-null UserId emitted by AccountService • Encapsulate these calls in private local functions
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16399 +/- ##
==========================================
+ Coverage 41.25% 41.39% +0.14%
==========================================
Files 3546 3546
Lines 102040 102049 +9
Branches 15308 15317 +9
==========================================
+ Hits 42099 42246 +147
+ Misses 58177 58037 -140
- Partials 1764 1766 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-Refactor-TS-Enums-to-be-const-objects merge changes from import related branch
…to PM-22143-Tools-Refactor-TS-Enums-to-be-const-objects merge changes from PM-24105
…PORT-ONLY' into PM-22143-Tools-Refactor-TS-Enums-to-be-const-objects merge changes from export related branch
harr1424
changed the title
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and all feature flags disabled. ✅ Fortunately, these BIT tests have passed! 🎉 |
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and the feature flag configuration used by ✅ Fortunately, these BIT tests have passed! 🎉 |
* Import related changes from PR #16399
harr1424
changed the title
|
| this.sendId = params.sendId; | ||
| this.type = parseInt(params.type, 10); | ||
| const sendTypeValue = parseInt(params.type, 10); | ||
| if (sendTypeValue === SendType.Text || sendTypeValue === SendType.File) { |
There was a problem hiding this comment.
[SendType.Text, SendType.File].indexOf(sendTypeValue) !== -1
could be
| this.sendListFiltersService.filters$, | ||
| ]) | ||
| .pipe(takeUntilDestroyed()) | ||
| .subscribe(([emptyList, noFilteredResults, currentFilter]) => { |
| } | ||
| } else { | ||
| this.title = "allSends"; | ||
| } |
There was a problem hiding this comment.
could be this to avoid nested if conditions
if (currentFilter?.sendType != null) {
const titleMap: Record<SendType, string> = {
[SendType.File]: 'fileSends',
[SendType.Text]: 'textSends'
};
this.title = titleMap[currentFilter.sendType] ?? 'allSends';
} else {
this.title = 'allSends';
}
| * Retrieves the symbolic name for a DatePreset value. | ||
| */ | ||
| export function nameOfDatePreset(value: DatePreset): keyof typeof DatePreset | undefined { | ||
| return namesByDatePreset.get(value); |
There was a problem hiding this comment.
consider throwing an error instead of returning undefined
|
Claude finished @harr1424's task —— View job Code Review CompleteThanks for your work on refactoring these enums to const objects per ADR-0025. I've reviewed all the changes and have the following feedback: SummaryThe refactoring successfully converts Send-related and export-related enums to const objects. The core pattern is correctly implemented with proper JSDoc documentation and runtime type guards. However, there are several findings that need attention, particularly around error handling, code organization, and addressing reviewer questions. FindingsFinding 1: Error thrown in QueryParams constructor may not be handled gracefully
Finding 2: DatePreset inconsistencies between files create maintenance burden
Finding 3: Unnecessary string type check in formattedDeletionDate getter
Finding 4: Action and SendState const objects are exported but only used internally
Finding 5: Nested conditionals in title assignment could be simplified
Finding 6: Getter with side effects needs resolution
Finding 7: Unused runtime type guards in send-add-edit-dialog.component
Finding 8: Type assertion in send.program.ts could be typed
Positive Observations
Action Items Summary
Addressed Review CommentsBased on recent review comments from Hinton and your responses:
Please let me know if you'd like me to help implement any of these changes or if you have questions about the recommendations. |
libs/tools/send/send-ui/src/add-edit/send-add-edit-dialog.component.ts
Outdated
Show resolved
Hide resolved
| export const DatePreset = Object.freeze({ | ||
| /** One-hour duration. */ | ||
| OneHour: 1, | ||
| /** One-day duration (24 hours). */ | ||
| OneDay: 24, | ||
| /** Two-day duration (48 hours). */ | ||
| TwoDays: 48, | ||
| /** Three-day duration (72 hours). */ | ||
| ThreeDays: 72, | ||
| /** Seven-day duration (168 hours). */ | ||
| SevenDays: 168, | ||
| /** Thirty-day duration (720 hours). */ | ||
| ThirtyDays: 720, | ||
| /** Custom date/time, provided by the user. */ | ||
| Custom: 0, | ||
| /** No expiration. */ | ||
| Never: "never", | ||
| } as const); |
There was a problem hiding this comment.
issue: This is duplicated. Can we extract this to a single file and share it in both places. OR if we consider this file to be deprecated, not modify it.
There was a problem hiding this comment.
@Hinton please let me know your preference here, if this file is deprecated my preference would be to not modify it.
There was a problem hiding this comment.
Yea that's fine with me. The file is deprecated and will be replaced with the new component so the value of updating it probably only results in additional QA effort.
apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.ts
Show resolved
Hide resolved
| /** An action that can be performed in the Send UI. */ | ||
| export const Action = Object.freeze({ | ||
| /** No action is currently active. */ | ||
| None: "", | ||
| /** The user is adding a new Send. */ | ||
| Add: "add", | ||
| /** The user is editing an existing Send. */ | ||
| Edit: "edit", | ||
| } as const); | ||
|
|
||
| /** An action that can be performed in the Send UI. */ | ||
| export type Action = (typeof Action)[keyof typeof Action]; |
There was a problem hiding this comment.
question: These are internal to this file, should they be exported?
There was a problem hiding this comment.
I've updated this file to not export these.
| Never = null, | ||
| } | ||
| /** A preset duration (in hours) for expiration/deletion. */ | ||
| export const DatePreset = Object.freeze({ |
There was a problem hiding this comment.
issue: This is internal to the file, should they be exported?
There was a problem hiding this comment.
I've updated this file and the related spec file.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22143
📔 Objective
This PR only contains work done to refactor the Send related enums and one export related enum.
Import related enums will be included in a separate PR, per suggestion in the discussion below:
Description
A decision was made to move away from using enums in TS and instead to use const objects.
Task
Refactor all tools enums into const objects.
For easier review it might be good to split Send and Import into separate PRs
When changing SendType, please be careful that it will likely break some navigation on the send-v2.component and maybe even desktop related as it doesn’t share the UI components yet
Remove all associated eslint disable statements
Context/Resources
PR enabling eslint rule
Ticket for enabling eslint rule
PR for contributing docs
⏰ Reminders before review
🦮 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