use undefined instead of null for most refs
#374
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors Vue component error handling and state management to use undefined instead of null for uninitialized ref values across the frontend codebase. The changes improve type safety and code consistency by updating error state variables, loading indicators, and conditional checks to follow a unified pattern with strict equality comparisons.
Key changes:
- Error and loading state refs changed from
Error | nulltoErrorwithundefinedas the default value - Conditional rendering updated to use truthy checks (
v-if="!onFetchError") or strict equality (=== undefined) - FetchError component's
retryprop changed from nullable to optional (retry?: (() => Promise<void>))
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| VaultList.vue | Updated error/loading refs and conditionals to use undefined; strict equality for vault checks |
| VaultDetails.vue | Refactored error state refs and checks; updated retry prop usage |
| UserProfile.vue | Changed error state ref and conditionals to use undefined |
| UnlockSuccess.vue | Updated error state and loading checks to use undefined |
| SignUserKeysDialog.vue | Refactored error state ref to use undefined |
| SearchInputGroup.vue | Updated selectedItem ref and conditionals to use strict equality with undefined |
| ResetUserAccountDialog.vue | Changed error state ref to use undefined |
| RegenerateSetupCodeDialog.vue | Updated error state ref to use undefined |
| ReactivateVaultDialog.vue | Refactored error state ref to use undefined |
| ManageSetupCode.vue | Changed error state and conditionals to use undefined |
| LegacyDeviceList.vue | Updated error state refs to use undefined |
| InitialSetup.vue | Refactored multiple error state refs to use undefined |
| GrantPermissionDialog.vue | Changed error state ref to use undefined |
| FetchError.vue | Updated retry prop from nullable to optional |
| DeviceList.vue | Refactored error state refs to use undefined |
| CreateVault.vue | Updated multiple error state refs and conditionals to use undefined |
| AuthenticatedMain.vue | Changed error state ref and checks to use undefined |
| AuditLog.vue | Updated error state ref to use undefined |
| ArchiveVaultDialog.vue | Refactored error state ref to use undefined |
| AdminSettings.vue | Changed multiple error state refs and complex conditionals to use undefined |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR consistently narrows nullable reactive error refs from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2024-07-27T13:14:40.963ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
frontend/src/components/CreateVault.vue (1)
265-267: Consider using explicitundefinedin the type for better type safety.The refs are declared as
ref<Error>()but initialized withundefinedand later assignedundefined(lines 297, 312, 326, 343, 375). For proper TypeScript type safety, these should beref<Error | undefined>()to accurately reflect that the value can be either an Error or undefined.Apply this diff:
-const onCreateError = ref<Error>(); -const onRecoverError = ref<Error>(); -const onDownloadTemplateError = ref<Error>(); +const onCreateError = ref<Error | undefined>(); +const onRecoverError = ref<Error | undefined>(); +const onDownloadTemplateError = ref<Error | undefined>();frontend/src/components/SignUserKeysDialog.vue (1)
113-121: Consider type-safe error handling in catch block.The
errorcaught in the catch block is typed asunknownin TypeScript. Assigning it directly toonSignError.valuewithout type checking reduces type safety.Apply this diff to add proper error type handling:
async function sign() { try { const newTrust = await wot.sign(props.user); emit('signed', newTrust); open.value = false; } catch (error) { - onSignError.value = error; + onSignError.value = error instanceof Error ? error : new Error(String(error)); } }frontend/src/components/GrantPermissionDialog.vue (2)
51-56: Error display now driven by truthiness; consider tightening Conflict/NotFound conditionUsing
v-if="onGrantPermissionError"aligns with the newundefined-based error ref and keeps the generic error message logic clear. If you want to make the Conflict/NotFound branch a bit more explicit (and avoid relying on two separate conditions), you could optionally gate it on the same truthy check:- <p v-if="onGrantPermissionError instanceof ConflictError || onGrantPermissionError instanceof NotFoundError" class="text-sm text-red-900 px-4 sm:px-6 pb-3 text-right bg-red-50"> + <p v-if="onGrantPermissionError && (onGrantPermissionError instanceof ConflictError || onGrantPermissionError instanceof NotFoundError)" class="text-sm text-red-900 px-4 sm:px-6 pb-3 text-right bg-red-50"> {{ t('common.reload') }} </p>
111-120: Error reset ingrantAccessis sound; optionally clear on dialog open to avoid stale errorsResetting
onGrantPermissionErrortoundefinedbefore thetryand normalizing any caught value to anErrorobject is solid and keeps template usage simple. If you want to avoid showing a previous error when the dialog is reopened (before the user retries), you could also clear the error inshow():function show() { - open.value = true; + onGrantPermissionError.value = undefined; + open.value = true; }frontend/src/components/FetchError.vue (1)
10-10: Minor: Remove trailing space in template condition.There's a trailing space before the closing quote in
v-if="retry !== undefined ".Apply this diff:
- <div v-if="retry !== undefined " class="mt-6"> + <div v-if="retry !== undefined" class="mt-6">frontend/src/components/ManageSetupCode.vue (1)
64-64: Optional: make theonFetchErrorundefinedability explicit
const onFetchError = ref<Error>();works fine and results in aRef<Error | undefined>at the type level. To make the intent a bit clearer to readers, you could consider spelling the union out explicitly:-const onFetchError = ref<Error>(); +const onFetchError = ref<Error | undefined>();Not required for correctness, just a minor readability/typing nicety.
frontend/src/components/UserProfile.vue (2)
2-7: Undefined-based loading/error gate matches new patternUsing
v-if="me === undefined"together withv-if="!onFetchError"cleanly separates “not loaded yet” from “failed to load” and aligns with the undefined-centric convention. Behavior (loading first, then error onceonFetchErroris set) looks unchanged and correct given current usage offetchData.
18-18: Email truthiness check subtly changes behavior for empty stringsSwitching from
me.email != nulltov-if="me.email"means an empty-string email will no longer render an empty<p>line. That’s likely preferable UX, but it’s a minor behavior change worth being aware of if any upstream can send''as email.frontend/src/components/SearchInputGroup.vue (2)
75-75: Optional: align watcher comparison with strict equality
if (value != undefined)works given your current usage, but for consistency with the rest of this component (which uses=== undefined/!== undefined) you might prefer:- if (value != undefined) { + if (value !== undefined) {Purely stylistic; behavior is unchanged as long as
selectedItemis only everTorundefined.
4-4: Consider normalizing Combobox updates to preventnullfrom reachingselectedItem
selectedItemis typed asshallowRef<T>()without explicit null/undefined, while the template assumes the non-undefined branch always holds a valid object (e.g.,selectedItem.pictureUrl,selectedItem.name). The@headlessui/vueCombobox v1.7.23 supports thenullableprop and will emitnullvia@update:model-valuewhen cleared and blurred. Ifnullableis added to the Combobox,selectedItemwould receivenull, bypass the=== undefinedchecks, and cause property access errors.To keep the code robust against null values, normalize the event payload and tighten the watcher comparison:
- <Combobox as="div" class="w-full" @update:model-value="item => selectedItem = item"> + <Combobox as="div" class="w-full" @update:model-value="item => selectedItem = item ?? undefined"> watch(selectedItem, (value) => { - if (value != undefined) { + if (value !== undefined) { nextTick(() => actionButton.value?.focus()); } });This ensures
selectedItemnever holdsnulland keeps the type and template assumptions safe.Also applies to: 73-76, 115-116
frontend/src/components/AdminSettings.vue (3)
2-3: Loading/error guard aligns with newundefinedsemantics; consider resettingonFetchErroron retrySwitching the initial guard to
admin/version/wot* === undefinedand using!onFetchErrorto distinguish loading vs error is consistent with the new pattern and looks correct. One small UX improvement: whenfetchDatais called again via the retry button,onFetchErroris never cleared before the new request, so the previous error message remains visible while the retry is in flight. If you want to show the loading state during retries, setonFetchError.value = undefinedat the start offetchData().Also applies to: 298-299, 364-387
252-252: Error refs + truthy checks work; consider explicitError | undefinedtypingThe move to
ref<Error>()plus clearing withundefinedand using truthiness in the template (onSaveError && !(onSaveError instanceof FormValidationFailedError),wotMaxDepthError,wotIdVerifyLenError) is logically sound and matches the rest of the PR.If TypeScript
strictNullChecksis enabled, assigningundefinedinto aRef<Error>can be noisy; you might want to make theundefinedpart of the type for clarity:-const onFetchError = ref<Error>(); -const onSaveError = ref<Error>(); -const wotMaxDepthError = ref<Error >(); -const wotIdVerifyLenError = ref<Error>(); +const onFetchError = ref<Error | undefined>(undefined); +const onSaveError = ref<Error | undefined>(undefined); +const wotMaxDepthError = ref<Error | undefined>(undefined); +const wotIdVerifyLenError = ref<Error | undefined>(undefined);The template logic would stay the same, but the types better document that “no error” is represented by
undefined.Also applies to: 298-302, 395-397, 426-426
398-398: Guard insaveWebOfTruststill uses== undefinedThe guard
if (admin.value == undefined || wotMaxDepth.value == undefined || wotIdVerifyLen.value == undefined) {is functionally fine, but it stands out since most of the file now uses strict
=== undefinedchecks. If the DTOs guarantee these values are nevernull, you could tighten this to=== undefinedfor consistency; ifnullis still a possible backend value, keeping== undefinedis correct, and a short comment explaining that intent might help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
frontend/src/components/AdminSettings.vue(6 hunks)frontend/src/components/ArchiveVaultDialog.vue(3 hunks)frontend/src/components/AuditLog.vue(4 hunks)frontend/src/components/AuthenticatedMain.vue(2 hunks)frontend/src/components/CreateVault.vue(11 hunks)frontend/src/components/DeviceList.vue(3 hunks)frontend/src/components/FetchError.vue(2 hunks)frontend/src/components/GrantPermissionDialog.vue(3 hunks)frontend/src/components/InitialSetup.vue(7 hunks)frontend/src/components/LegacyDeviceList.vue(3 hunks)frontend/src/components/ManageSetupCode.vue(4 hunks)frontend/src/components/ReactivateVaultDialog.vue(3 hunks)frontend/src/components/RegenerateSetupCodeDialog.vue(3 hunks)frontend/src/components/ResetUserAccountDialog.vue(3 hunks)frontend/src/components/SearchInputGroup.vue(4 hunks)frontend/src/components/SignUserKeysDialog.vue(2 hunks)frontend/src/components/UnlockSuccess.vue(2 hunks)frontend/src/components/UserProfile.vue(5 hunks)frontend/src/components/VaultDetails.vue(7 hunks)frontend/src/components/VaultList.vue(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
Learnt from: infeo
Repo: cryptomator/hub PR: 0
File: :0-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: In `LicenseAlert.vue`, the null/undefined check for `license.licensedSeats` and `license.usedSeats` is handled by first checking the `license` object itself.
📚 Learning: 2024-07-27T13:14:40.963Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
Applied to files:
frontend/src/components/UnlockSuccess.vuefrontend/src/components/SignUserKeysDialog.vuefrontend/src/components/CreateVault.vuefrontend/src/components/VaultDetails.vuefrontend/src/components/InitialSetup.vue
📚 Learning: 2024-07-27T13:14:40.963Z
Learnt from: infeo
Repo: cryptomator/hub PR: 0
File: :0-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: In `LicenseAlert.vue`, the null/undefined check for `license.licensedSeats` and `license.usedSeats` is handled by first checking the `license` object itself.
Applied to files:
frontend/src/components/AdminSettings.vuefrontend/src/components/CreateVault.vuefrontend/src/components/VaultDetails.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Run Tests
🔇 Additional comments (42)
frontend/src/components/ResetUserAccountDialog.vue (1)
37-38: LGTM!The null-to-undefined refactoring is correctly applied:
ref<Error>()properly initializes toundefined- Truthiness check
v-if="onResetError"correctly handles theError | undefinedstate- Reset to
undefinedbefore the async operation is consistent with the new patternAlso applies to: 61-61, 84-84
frontend/src/components/AuditLog.vue (1)
3-8: LGTM!The error state handling is correctly refactored:
ref<Error>()with implicit undefined initialization!onFetchErrorandv-if="onFetchError"truthiness checks work correctly- Reset to
undefinedinfetchDatais consistentNote: The
== nullchecks invalidateDateFilterValueusage (lines 269-280) are correctly left unchanged since that function explicitly returnsDate | null.Also applies to: 201-201, 257-257, 333-333
frontend/src/components/UnlockSuccess.vue (1)
6-12: LGTM!The refactoring is correctly applied:
me === undefinedappropriately checks for the uninitialized/loading state!onFetchErrortruthiness check works correctly for the error display logicref<Error>()and reset toundefinedfollow the PR pattern consistentlyAlso applies to: 124-124, 129-129
frontend/src/components/DeviceList.vue (1)
2-9: LGTM!The refactoring is correctly implemented:
me === undefinedand!onFetchErrorproperly handle loading/error statesref<Record<string, Error>>({})with truthiness checkonRemoveDeviceError[device.id]works correctly (property access returnsundefinedfor missing keys)- The
deleteoperator (line 127) properly removes entries, making subsequent truthiness checks returnundefinedAlso applies to: 81-85, 108-109, 116-116
frontend/src/components/LegacyDeviceList.vue (1)
2-9: LGTM!The refactoring follows the same correct pattern as
DeviceList.vue:
- Loading/error state checks using
=== undefinedand truthinessref<Record<string, Error>>({})for per-device error tracking- Consistent reset to
undefinedinfetchDataAlso applies to: 82-86, 108-109, 116-116
frontend/src/components/ArchiveVaultDialog.vue (1)
38-38: LGTM! Clean null-to-undefined refactoring.The error state refactoring is correct and internally consistent:
- The ref type
ref<Error>()correctly infersRef<Error | undefined>in Vue's type system- The truthiness check
v-if="onArchiveVaultError"is safe since Error objects are always truthy and undefined is falsy- The reset to
undefinedaligns with the initialization behaviorThis follows idiomatic Vue patterns and preserves all existing functionality.
Also applies to: 61-61, 81-81
frontend/src/components/AuthenticatedMain.vue (3)
2-10: LGTM!The loading/error flow logic is correct:
me === undefinedappropriately guards the loading/error state!onFetchErrorcorrectly leverages truthiness to distinguish between loading (undefined → falsy) and error (Error → truthy)- When
FetchErroris rendered,onFetchErroris guaranteed to be anErrorinstance
31-32: Consistent initialization pattern.Both refs correctly use the no-argument form, allowing TypeScript to infer
Ref<T | undefined>. This is idiomatic Vue 3 and aligns well with the PR's goal of standardizing onundefinedfor uninitialized state.
36-44: LGTM!The error reset to
undefinedon line 37 correctly clears any previous error state before retrying. The error handling logic properly wraps non-Error types, maintaining type safety.frontend/src/components/ReactivateVaultDialog.vue (1)
38-38: LGTM! Clean null-to-undefined refactoring.The error state refactoring is consistent and correct:
ref<Error>()implicitly createsRef<Error | undefined>(Vue 3 standard)- Truthiness check in template works identically (both
nullandundefinedare falsy)- Error reset with
undefinedaligns with the ref typeThe changes maintain type safety and runtime behavior while following the PR's migration pattern.
Also applies to: 61-61, 81-81
frontend/src/components/InitialSetup.vue (3)
2-2: LGTM! Template conditionals correctly updated.The template changes consistently implement the null-to-undefined refactoring:
- Line 2 uses strict equality (
me !== undefined) for explicit presence checking- Lines 6, 99, and 141 use truthy checks for error state rendering, which correctly treat
undefinedas falsy andErrorobjects as truthyAlso applies to: 6-6, 99-99, 141-141
218-220: LGTM! Error ref declarations follow correct Vue 3 typing.The error refs now use
ref<Error>()which correctly createsRef<Error | undefined>types in Vue 3, allowing both Error objects and undefined while initializing to undefined by default.
240-240: LGTM! Error resets consistently use undefined.All error state resets now consistently use
undefinedinstead ofnull, aligning with the ref initialization semantics and the PR's refactoring objectives.Also applies to: 258-258, 281-281
frontend/src/components/RegenerateSetupCodeDialog.vue (2)
37-39: Template error condition correctly switched to truthy checkUsing
v-if="onRegenerateError"together with{{ onRegenerateError.message }}is safe here, since the block only renders when the ref holds anErrorinstance and remains hidden when it isundefined. This aligns cleanly with the new undefined-based error state pattern.
116-149:onRegenerateErrorref and reset follow the new undefined semanticsDeclaring
const onRegenerateError = ref<Error>();and resetting withonRegenerateError.value = undefined;gives a falsy initial/cleared state while thecatchblock always stores a concreteError. This matches the PR’s move away fromnulland keeps both script and template logic straightforward.frontend/src/components/CreateVault.vue (4)
32-35: LGTM! Truthiness checks are appropriate for error display.The template changes from explicit null checks to truthiness checks work correctly. Error objects are always truthy when present, and the nested
instanceofchecks for specific error types remain intact.Also applies to: 81-88, 159-166, 214-214
231-231: LGTM! Good cleanup of unused import.Removing the unused
computedimport improves code clarity.
297-297: LGTM! Error resets to undefined are consistent with the refactor.Resetting error refs to
undefinedinstead ofnullaligns with the PR objectives and works correctly with the truthiness checks in the template.Also applies to: 312-312, 326-326, 343-343, 375-375
378-382: LGTM! Strict undefined check is more explicit.The change from loose equality (
!= null) to strict undefined check (!== undefined) is more explicit and consistent with the PR's refactoring goals.frontend/src/components/SignUserKeysDialog.vue (1)
47-47: LGTM! Refactoring aligns with PR objectives.The changes correctly implement the undefined-based error handling approach:
- The ref type change from
ref<Error | null>toref<Error>initializes the value asundefined- The template conditional change from explicit null check to truthiness check (
v-if="onSignError") works correctly sinceundefinedis falsyAlso applies to: 85-85
frontend/src/components/GrantPermissionDialog.vue (1)
78-81: Error ref type and initialization are consistent with the PR’s undefined-based patternSwitching to
const onGrantPermissionError = ref<Error>();and treating “no error” as theundefinedinitial state matches the PR’s goal of removingnullfrom these refs and works cleanly with the template’s truthiness checks.frontend/src/components/FetchError.vue (1)
27-27: LGTM: Optional prop pattern is correct.The change from
retry: (() => Promise<void>) | nulltoretry?: (() => Promise<void>)properly uses TypeScript's optional parameter syntax, which is the idiomatic way to handle optional props in Vue 3 with TypeScript.frontend/src/components/VaultDetails.vue (5)
2-7: LGTM: Undefined checks are consistent.The loading and error state handling correctly uses strict undefined comparison for
vaultand truthy check foronFetchError, aligning with the PR's objective to standardize on undefined-based state management.
214-214: LGTM: Remeda import supports data transformation.The remeda library is used on line 303 to convert the members array into a Record, which aligns with the existing
memberstype definition at line 270.
246-250: LGTM: Error refs now use non-nullable types.Changing from
ref<Error | null>toref<Error>with undefined assignment is consistent with the PR's migration strategy and improves type safety by relying on undefined for the "no error" state.
286-286: LGTM: Error state resets use undefined.Resetting error states to
undefinedinstead ofnullis consistent with the non-nullable error ref types and the PR's migration objective.Also applies to: 371-371
303-303: LGTM: Remeda indexBy correctly transforms members array.The use of
R.indexByto convert the members array into aRecord<string, MemberDto>keyed by member id is appropriate, as evidenced by the existing usages throughout the file (lines 379, 466, 481, 496) that access members by id.frontend/src/components/VaultList.vue (5)
2-9: LGTM: Loading and error state logic migrated to undefined.The use of falsy checks (
!vaults,!onFetchError) in the template is appropriate for the undefined-based state management and aligns with the non-nullable error ref type.
74-110: LGTM: Filtered vaults rendering uses appropriate truthiness checks.The conditional rendering logic correctly handles undefined states for
filteredVaultsby checking both presence and length, ensuring proper display of vault list and empty states.
112-112: LGTM: SlideOver close resets to undefined.Resetting
selectedVaulttoundefinedon close is consistent with the non-nullable ref type and the PR's undefined-based state management pattern.
133-138: LGTM: Refs use non-nullable types.The error and selected vault refs now use
ref<Error>andref<VaultDto>without null unions, relying on undefined for the "not set" state, which improves type safety and consistency.
179-179: LGTM: Undefined assignment and strict equality checks.Resetting
onFetchErrortoundefined(line 179) and using strict equality=== undefined(line 216) are consistent with the PR's migration strategy and improve code clarity.Also applies to: 216-216
frontend/src/components/ManageSetupCode.vue (4)
2-3: Loading/error gating withundefinedlooks consistentUsing
setupCode === undefinedfor the loading branch and!onFetchErrorto distinguish loading vs error aligns well with the newundefined‑based state model; behavior stays correct and clear.
44-44: Good guard on dialog rendering withsetupCode !== undefinedGating
RegenerateSetupCodeDialogon bothregeneratingSetupCodeandsetupCode !== undefinedprevents the dialog from mounting without a known setup code and matches howshowRegenerateSetupCodeDialogis only reachable from the loaded view.
69-69: Error reset before fetch is correctResetting
onFetchError.valuetoundefinedat the start offetchDatacorrectly switches the template back into the loading state before each retry.
89-89: Defensiveundefinedguard before copying is soundThe explicit
setupCode.value === undefinedcheck before copying is a solid invariant guard; it should never trigger in normal UI flow, but it protects against misuse and keeps thestring | undefinedtype honest.frontend/src/components/UserProfile.vue (2)
53-57: Version visibility guard is consistent and safe
v-if="version !== undefined"is consistent with themecheck and safely prevents accessing properties on an unset ref. Givenversionis only ever assigned a properVersionDto, this should be behaviorally equivalent to the previous null check.
91-108: onFetchError ref/reset aligns with template truthiness checksInitializing
onFetchErrorviaref<Error>()and clearing it withonFetchError.value = undefinedbefore eachfetchDatarun fits the!onFetchErrorcondition in the template and avoids stale errors staying visible during retries. Setting it to anErrorinstance in the catch block keeps the template logic straightforward and looks correct.frontend/src/components/SearchInputGroup.vue (3)
7-18: Template undefined-based empty state looks coherentUsing
selectedItem === undefinedto gate the icon, input, and options visibility is consistent with initializing and resettingselectedItemtoundefined, and keeps the “no selection” UI state cleanly separated from the “item selected” state. This matches the PR’s shift away fromnulland looks good.
29-35: Clear and action button behavior correctly mirrors selection stateThe clear button (
v-if="selectedItem !== undefined") and the action button’s disabled state (:disabled="selectedItem === undefined") are aligned with the new sentinel and withonAction()’sif (selectedItem.value)guard, so the user can only trigger the action when a real item is selected and can always clear that selection. No issues here.
115-115: Reset implementation matches new undefined sentinel
reset()assigningselectedItem.value = undefinedand clearingqueryis consistent with the template and button logic that useundefinedas the empty state. This keeps the component’s internal and visual state in sync after an action or manual clear.frontend/src/components/AdminSettings.vue (1)
75-75: License sections gated byremainingSeats !== undefinedlook consistentUsing
v-if="admin.hasLicense && remainingSeats !== undefined"/!admin.hasLicense && remainingSeats !== undefinedmatches theremainingSeatscomputed (undefined meaning “no admin/seat data yet”) and avoids rendering the license UI until seat information is available. This is consistent with the pattern of checking the parent object rather than nested seat fields used elsewhere (e.g., LicenseAlert.vue).Based on learnings, this matches the existing license/seat visibility approach.
Also applies to: 143-143, 329-335
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/SearchInputGroup.vue (1)
72-116: AlignselectedItem’s TypeScript type with the undefined sentinel and make checks explicitYou’re now using
undefinedas the “no selection” state (selectedItem.value = undefined,value !== undefined), butselectedItemis typed asshallowRef<T>(). That means:
- At runtime,
selectedItem.valueisT | undefined, but at the type level it’s justT.- The
if (value !== undefined)in the watcher and the sentinel usage may conflict with stricter TS settings (e.g., “this condition will always be true” diagnostics), and TS won’t narrow correctly.To make the intent explicit and improve type-safety and consistency, consider:
- Typing the ref as
T | undefined.- Making the action handler check for
!== undefinedas well.For example:
-const selectedItem = shallowRef<T>(); -watch(selectedItem, (value) => { - if (value !== undefined) { +const selectedItem = shallowRef<T | undefined>(); +watch(selectedItem, (value) => { + if (value !== undefined) { nextTick(() => actionButton.value?.focus()); } }); @@ function onAction() { - if (selectedItem.value) { - emit('action', selectedItem.value); + if (selectedItem.value !== undefined) { + emit('action', selectedItem.value); reset(); } } @@ function reset() { selectedItem.value = undefined; query.value = ''; }This keeps the runtime behavior the same while making the “undefined = no selection” contract explicit in the types and conditions.
Please verify with your current TypeScript config (especially
strictNullChecks) that the existingwatchcallback andshallowRef<T>()don’t already trigger diagnostics, and that the suggestedT | undefinedtyping compiles cleanly in your setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/SearchInputGroup.vue(4 hunks)frontend/src/components/VaultDetails.vue(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/VaultDetails.vue
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
Learnt from: infeo
Repo: cryptomator/hub PR: 0
File: :0-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: In `LicenseAlert.vue`, the null/undefined check for `license.licensedSeats` and `license.usedSeats` is handled by first checking the `license` object itself.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (1)
frontend/src/components/SearchInputGroup.vue (1)
7-36: Template: undefined sentinel usage looks consistent and correctThe switch from
nulltoundefinedas the “no selection” sentinel is applied consistently here:
- Icon, input, options, clear button, and action button all gate on
selectedItem === undefined/!== undefined.- This matches the intended UX: input/options visible only with no selection, clear button and action only when a selection exists.
I don’t see behavioral regressions introduced by these changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/RecoverVaultDialog.vue (1)
72-103: Consider wiring form validation failures intoonVaultRecoverErrorinstead of throwingThe change to
const onVaultRecoverError = ref<Error>();and resetting it withonVaultRecoverError.value = undefined;inrecoverVaultis consistent with the PR’s null→undefined pattern.However, the template has a special branch for
FormValidationFailedError, whilevalidateRecoveryKeycurrently throws that error instead of assigning it to the ref. SinceonVaultRecoverErroris only set insiderecoverVault’scatch, the dedicatedFormValidationFailedErrorUI path may never be hit unless some other code throws that error type during recovery.If the intent is to surface form-validation failures via this dialog’s error area, consider assigning the error to the ref and returning early instead of throwing:
async function validateRecoveryKey() { if (!form.value?.checkValidity()) { - throw new FormValidationFailedError(); + onVaultRecoverError.value = new FormValidationFailedError(); + return; } await recoverVault(); }This keeps the new
undefined-based error state, makes the template’sFormValidationFailedErrorbranch reachable, and avoids an unhandled rejection on invalid form input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ClaimVaultOwnershipDialog.vue(3 hunks)frontend/src/components/DownloadVaultTemplateDialog.vue(3 hunks)frontend/src/components/EditVaultMetadataDialog.vue(3 hunks)frontend/src/components/RecoverVaultDialog.vue(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
Learnt from: infeo
Repo: cryptomator/hub PR: 0
File: :0-0
Timestamp: 2024-07-27T13:14:40.963Z
Learning: In `LicenseAlert.vue`, the null/undefined check for `license.licensedSeats` and `license.usedSeats` is handled by first checking the `license` object itself.
📚 Learning: 2024-07-27T13:14:40.963Z
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
Applied to files:
frontend/src/components/RecoverVaultDialog.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (8)
frontend/src/components/EditVaultMetadataDialog.vue (1)
48-48: LGTM! Clean refactoring toundefined.The changes consistently replace
nullwithundefinedfor error state management:
- Line 84: Type narrowed from
Error | nulltoError(implicitlyError | undefined)- Line 109: Reset uses
undefinedinstead ofnull- Line 48: Truthy check is simpler and semantically equivalent
This aligns with TypeScript best practices where
undefinedis preferred for optional values.Also applies to: 84-84, 109-109
frontend/src/components/DownloadVaultTemplateDialog.vue (3)
37-39: LGTM! Truthy check is appropriate for error display.The template conditional correctly relies on the truthiness of
onDownloadError. Whenundefined, the error message is hidden; when an Error object is assigned, the message displays.
64-64: LGTM! Error ref declaration follows PR pattern.The declaration
ref<Error>()correctly relies on TypeScript's overload to inferRef<Error | undefined>, makingundefinedthe initial value. This is consistent with the broader PR refactor.
84-92: LGTM! Error handling correctly usesundefinedfor reset.The error state is properly cleared with
undefinedbefore attempting the download, and Error objects are correctly assigned in the catch block. This aligns with the PR's refactor objectives.frontend/src/components/ClaimVaultOwnershipDialog.vue (3)
38-48: Truthy error check is consistent with newundefinedsemanticsUsing
v-if="onAuthenticationError"matches the newError | undefinedbehavior: the block renders only when anErrorinstance is present, and allinstanceofbranches remain correct. No behavior regression here.
77-77:onAuthenticationErrorref initialization aligns with usageDeclaring
const onAuthenticationError = ref<Error>();and only assigning actualErrorinstances (or leaving it unset) matches the template’s truthy checks andinstanceofconditions. This is consistent with the PR-wide null→undefined pattern.
99-115: Resetting error state withundefinedbefore auth attempt is correctClearing
onAuthenticationError.valuetoundefinedat the start and then assigning a concreteErrorin the catch block cleanly separates “no error” vs. “have error” states without changing control flow. Behavior is preserved while aligning with the new convention.frontend/src/components/RecoverVaultDialog.vue (1)
35-42:v-if="onVaultRecoverError"truthiness check matches the new undefined-based error stateUsing a simple truthiness check here is correct now that
onVaultRecoverErroris cleared toundefinedand only ever holdsErrorinstances when set. The nestedinstanceof FormValidationFailedErrorvs generic error messages remain valid under this guard.
Co-authored-by: mindmonk <[email protected]>
This pull request refactors error handling and loading state checks across multiple frontend Vue components to use
undefinedinstead ofnullfor uninitialized values. It also updates conditional rendering logic to use strict equality checks and truthy evaluations, improving code consistency and reducing potential bugs related to falsy values. The changes affect error state variables, loading indicators, and some computed properties, streamlining the codebase for better readability and maintainability.Error handling and state management improvements:
All error state refs (such as
onFetchError,onSaveError,onCreateError, etc.) are changed fromError | nulltoError, and their assignments now useundefinedinstead ofnull. Related conditional checks are updated to use truthy evaluations (e.g.,v-if="onFetchError"instead ofv-if="onFetchError != null"). (frontend/src/components/AdminSettings.vue,frontend/src/components/ArchiveVaultDialog.vue,frontend/src/components/AuditLog.vue,frontend/src/components/AuthenticatedMain.vue,frontend/src/components/CreateVault.vue,frontend/src/components/DeviceList.vue) [1] [2] [3] [4] [5] [6]Error state reset logic now consistently uses
undefinedinstead ofnullwhen clearing errors before new operations. (frontend/src/components/AdminSettings.vue,frontend/src/components/ArchiveVaultDialog.vue,frontend/src/components/AuditLog.vue,frontend/src/components/AuthenticatedMain.vue,frontend/src/components/CreateVault.vue,frontend/src/components/DeviceList.vue) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Conditional rendering and strict equality:
Loading and data presence checks are updated to use strict equality (
=== undefined) instead of loose equality (== null), and loading/error conditions now use truthy checks for error states. (frontend/src/components/AdminSettings.vue,frontend/src/components/AuthenticatedMain.vue,frontend/src/components/DeviceList.vue,frontend/src/components/FetchError.vue) [1] [2] [3] [4]License and seat-related section visibility now relies on strict checks for
undefinedvalues instead ofnull. (frontend/src/components/AdminSettings.vue) [1] [2]Prop type and interface updates:
retryprop inFetchError.vueis updated to be optional (retry?: (() => Promise<void>)) instead of nullable, aligning with the new error handling approach. (frontend/src/components/FetchError.vue)Miscellaneous code cleanups:
!== undefinedfor blob checks, are included to further improve code clarity. (frontend/src/components/CreateVault.vue) [1] [2]These changes collectively make error and loading state handling more robust and consistent throughout the frontend codebase.