Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR factors emergency council selection, validation, and recovery-key splitting into a new Vue 3 component, EmergencyAccessSetup.vue. CreateVault, EmergencyAccessDialog, GrantEmergencyAccessDialog, and other callers now mount EmergencyAccessSetup and delegate validation/splitting to it; local council-selection state, redundancy checks, and splitting helpers were removed. EmergencyScenarioVisualization was refactored to a slot-lane layout. Vault creation and grant flows now consume requiredEmergencyKeyShares/emergencyKeyShares produced by EmergencyAccessSetup. Minor styling tweaks applied to MultiUserSelectInputGroup and conditional rendering updated where settings are required. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue (1)
55-55: Remove unusedisReadonlyimport.The
isReadonlyfunction is imported from Vue but never used in this component.🧹 Proposed fix
-import { ArrowUturnLeftIcon, ExclamationTriangleIcon } from '@heroicons/vue/24/solid'; -import { useId, computed, onMounted, ref, isReadonly } from 'vue'; +import { ArrowUturnLeftIcon, ExclamationTriangleIcon } from '@heroicons/vue/24/solid'; +import { useId, computed, onMounted, ref } from 'vue';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue` at line 55, Remove the unused isReadonly import from the Vue import list in the component; update the import statement that currently reads "import { useId, computed, onMounted, ref, isReadonly } from 'vue';" to exclude isReadonly so it becomes "import { useId, computed, onMounted, ref } from 'vue';" and ensure no other references to isReadonly exist in EmergencyAccessSetup.vue.frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue (1)
63-63: Consider usingreffortimeoutIdfor consistency.
timeoutIdis declared as a plainletvariable whilerandomSelectionIntervalusesref. Both work correctly, but usingreffor both would be more consistent with Vue's reactive patterns in this component.♻️ Optional: Use ref for timeoutId
-let timeoutId: ReturnType<typeof setTimeout> | undefined; +const timeoutId = ref<ReturnType<typeof setTimeout>>();Then update usage:
- if (timeoutId !== undefined) { - clearTimeout(timeoutId); + if (timeoutId.value !== undefined) { + clearTimeout(timeoutId.value); } - timeoutId = setTimeout(() => { + timeoutId.value = setTimeout(() => { loadingCouncilSelection.value = false; - timeoutId = undefined; + timeoutId.value = undefined; }, 100);Also applies to: 122-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue` at line 63, Convert the plain let timeoutId to a Vue ref for consistency with the existing reactive pattern (like randomSelectionInterval): replace the top-level declaration of timeoutId with a ref (e.g., const timeoutId = ref<ReturnType<typeof setTimeout> | undefined>(undefined)) and then update all usages where timeoutId is read, assigned, cleared (clearTimeout) to access or set timeoutId.value; ensure functions that set or clear the timer (the same places referenced around lines 122-128 and where randomSelectionInterval is used) use timeoutId.value so reactivity/consistency is maintained.frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue (1)
62-62: Consider defaulting totruewhenemergencyAccessSetupis not yet mounted.The button uses
emergencyAccessSetup?.hasValidationErrorswhich evaluates toundefined(falsy) before the component mounts, potentially allowing the button to be briefly enabled. While the dialog transition likely prevents immediate user interaction, an explicit default would be safer.🛡️ Safer disabled state
- :disabled="emergencyAccessSetup?.hasValidationErrors" + :disabled="emergencyAccessSetup?.hasValidationErrors ?? true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue` at line 62, The disabled binding currently uses emergencyAccessSetup?.hasValidationErrors which is undefined before mount and can briefly enable the button; change the binding to explicitly default to true when emergencyAccessSetup is not yet available (e.g., use a nullish-coalescing or ternary to evaluate emergencyAccessSetup?.hasValidationErrors and fall back to true) so that the button remains disabled until the setup object is mounted; update the template reference to emergencyAccessSetup and hasValidationErrors accordingly.frontend/src/components/CreateVault.vue (1)
140-140: Same optional chaining consideration as GrantEmergencyAccessDialog.Consider using
emergencyAccessSetup?.hasValidationErrors ?? trueto ensure the button is disabled before the component mounts.🛡️ Safer disabled state
- :disabled="emergencyAccessSetup?.hasValidationErrors || processing" + :disabled="(emergencyAccessSetup?.hasValidationErrors ?? true) || processing"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CreateVault.vue` at line 140, Replace the current disabled expression that uses optional chaining with a safe default so the button is disabled before the component mounts: change the binding that references emergencyAccessSetup?.hasValidationErrors (in CreateVault.vue) to use a nullish-coalescing default true for missing state and then OR with processing — i.e., use emergencyAccessSetup?.hasValidationErrors ?? true combined with processing so the button remains disabled until validity is known.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue`:
- Around line 40-44: The template is passing an undeclared prop :min-members to
EmergencyScenarioVisualization; remove the :min-members="minMembers" attribute
from the EmergencyScenarioVisualization usage (leave
:selected-users="emergencyCouncilMembers" and
:required-key-shares="requiredKeyShares" unchanged) so only the declared props
selectedUsers and requiredKeyShares are passed and Vue will not emit a non-prop
attribute warning.
---
Nitpick comments:
In `@frontend/src/components/CreateVault.vue`:
- Line 140: Replace the current disabled expression that uses optional chaining
with a safe default so the button is disabled before the component mounts:
change the binding that references emergencyAccessSetup?.hasValidationErrors (in
CreateVault.vue) to use a nullish-coalescing default true for missing state and
then OR with processing — i.e., use emergencyAccessSetup?.hasValidationErrors ??
true combined with processing so the button remains disabled until validity is
known.
In `@frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue`:
- Line 55: Remove the unused isReadonly import from the Vue import list in the
component; update the import statement that currently reads "import { useId,
computed, onMounted, ref, isReadonly } from 'vue';" to exclude isReadonly so it
becomes "import { useId, computed, onMounted, ref } from 'vue';" and ensure no
other references to isReadonly exist in EmergencyAccessSetup.vue.
In `@frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue`:
- Line 63: Convert the plain let timeoutId to a Vue ref for consistency with the
existing reactive pattern (like randomSelectionInterval): replace the top-level
declaration of timeoutId with a ref (e.g., const timeoutId =
ref<ReturnType<typeof setTimeout> | undefined>(undefined)) and then update all
usages where timeoutId is read, assigned, cleared (clearTimeout) to access or
set timeoutId.value; ensure functions that set or clear the timer (the same
places referenced around lines 122-128 and where randomSelectionInterval is
used) use timeoutId.value so reactivity/consistency is maintained.
In `@frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue`:
- Line 62: The disabled binding currently uses
emergencyAccessSetup?.hasValidationErrors which is undefined before mount and
can briefly enable the button; change the binding to explicitly default to true
when emergencyAccessSetup is not yet available (e.g., use a nullish-coalescing
or ternary to evaluate emergencyAccessSetup?.hasValidationErrors and fall back
to true) so that the button remains disabled until the setup object is mounted;
update the template reference to emergencyAccessSetup and hasValidationErrors
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c36cb7ee-1db9-4a76-b460-a974a19c33d4
📒 Files selected for processing (6)
frontend/src/components/CreateVault.vuefrontend/src/components/MultiUserSelectInputGroup.vuefrontend/src/components/emergencyaccess/EmergencyAccessDialog.vuefrontend/src/components/emergencyaccess/EmergencyAccessSetup.vuefrontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vuefrontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the emergency access configuration flows by extracting a reusable EmergencyAccessSetup component and integrating it into vault creation and emergency access dialogs, while also improving related UI components.
Changes:
- Introduced
EmergencyAccessSetupto centralize emergency council selection, validation, and key-splitting. - Replaced duplicated inline emergency access UI/logic in
CreateVault.vue,GrantEmergencyAccessDialog.vue, andEmergencyAccessDialog.vue. - Updated emergency access visualization/UI states (
EmergencyScenarioVisualization.vue,MultiUserSelectInputGroup.vue) for improved UX.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue | New shared component encapsulating emergency access council selection + validation + key splitting. |
| frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue | Replaced inline council/key-share logic with EmergencyAccessSetup. |
| frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue | Replaced council-change UI with EmergencyAccessSetup and delegated validation/state. |
| frontend/src/components/CreateVault.vue | Delegated emergency access setup/validation/splitting to EmergencyAccessSetup. |
| frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue | Refactored visualization rendering/animation and interval logic. |
| frontend/src/components/MultiUserSelectInputGroup.vue | Improved readonly/disabled styling and pill layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue
Outdated
Show resolved
Hide resolved
frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue
Show resolved
Hide resolved
frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
Outdated
Show resolved
Hide resolved
frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue
Outdated
Show resolved
Hide resolved
frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue
Show resolved
Hide resolved
Contributor
|
@overheadhunter Is it intentional that the Emergency Access Vault Council can no longer choose its own members when “Let vault owners choose different keyholders” is disabled? My understanding was that this restriction should only apply to vault owners, as also described in the text. |
Contributor
|
@tobihagemann I've opened a new pull request, #429, to work on those changes. Once the pull request is ready, I'll request review from you. |
Member
Author
good catch, fixed in latest version |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/CreateVault.vue`:
- Around line 344-349: When loadDefaultEmergencyAccessSettings catches an error
from backend.settings.get(), don't leave settings.value undefined (which is
treated as "disabled"); instead either set settings.value to a safe sentinel
that enforces emergency access (e.g., an object with emergencyAccessEnabled:
true) or set a settingsLoadError flag and make the create-vault flow check that
flag and block progression until settings are successfully reloaded. Update
loadDefaultEmergencyAccessSettings to assign that sentinel/flag on error and
update the downstream checks that read settings.value (the vault creation flow
in CreateVault.vue) to treat the sentinel/flag as "must block and retry" rather
than "disabled".
In `@frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue`:
- Around line 740-742: Split the logic that treats COUNCIL_CHANGE as unsupported
when there are too few new members: first validate process.type ===
'COUNCIL_CHANGE' and then explicitly check that councilMembers.value has at
least process.details.newRequiredKeyShares (and that all selected councilMembers
resolve to activated users); if that check fails throw a dedicated "insufficient
council members" (or similarly named) error instead of falling through to the
generic unsupported-type path so callers can diagnose completion failures;
update the block around EmergencyAccess.split and
backend.vaults.createOrUpdateVault to only execute when both the type is
COUNCIL_CHANGE and the sufficiency/activation check passes.
In `@frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue`:
- Around line 67-77: The component currently always overwrites the displayed
requiredKeyShares from backend.settings.get() inside initialize(), which
prevents callers from injecting a process-specific stored value; add an optional
prop (e.g., requiredKeyShares or initialRequiredKeyShares) to the defineProps
list and default to undefined, then change initialize() and the code at the
other spot that sets requiredKeyShares (references: initialize(),
requiredKeyShares) to only read backend.settings.get() when that prop is
undefined; callers rendering an existing council-change process should pass the
stored newRequiredKeyShares via this new prop so the view shows the process
value instead of the live default.
- Around line 125-136: The split function currently only rejects when
emergencyCouncilMembers.value.length < requiredKeyShares.value, allowing callers
to bypass the UI-enforced minimum; update split (the async function split and
its checks around requiredKeyShares and emergencyCouncilMembers) to also
validate against minMembers (e.g., if emergencyCouncilMembers.value.length <
minMembers.value) and throw the same localized error (use
t('grantEmergencyAccessDialog.error.tooFewCouncilMembers') or the appropriate
i18n key) so the API enforces the component's minMembers rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceadd285-473a-46df-990d-96504659d62c
📒 Files selected for processing (5)
frontend/src/components/CreateVault.vuefrontend/src/components/VaultDetails.vuefrontend/src/components/emergencyaccess/EmergencyAccessDialog.vuefrontend/src/components/emergencyaccess/EmergencyAccessSetup.vuefrontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/CreateVault.vue (1)
109-140:⚠️ Potential issue | 🟡 MinorMirror the child validation check inside the submit handler.
Line 140 disables the button, but the form can still be submitted from inside
EmergencyAccessSetup, and Line 436 will callsplit()even while validation errors are present. Return early here before invoking the child API so keyboard/programmatic submits can’t bypass the validation state.💡 Suggested change
async function validateVaultEmergencyAccess() { onCreateError.value = undefined; if (!emergencyAccessSetup.value || !vaultKeys.value) { onCreateError.value = new Error('Invalid state.'); return; } + if (emergencyAccessSetup.value.hasValidationErrors) { + return; + } processing.value = true; try {Also applies to: 427-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CreateVault.vue` around lines 109 - 140, The submit handler validateVaultEmergencyAccess() must mirror the disabled-button guard by checking the EmergencyAccessSetup ref for existence and for hasValidationErrors and return early if validation fails; update validateVaultEmergencyAccess() to read this.$refs.emergencyAccessSetup (or the Composition ref emergencyAccessSetup) and if falsy or emergencyAccessSetup.hasValidationErrors is true, do not call the child API (e.g., split()) or proceed with processing. Ensure the early-return is placed before any invocation of EmergencyAccessSetup methods (like split()) so keyboard/programmatic submits cannot bypass validation.
♻️ Duplicate comments (1)
frontend/src/components/CreateVault.vue (1)
344-349:⚠️ Potential issue | 🟠 MajorDon't fail open when emergency-access settings cannot be loaded.
Line 348 only logs the failure. If
backend.settings.get()rejects,settings.valuestaysundefined, andvalidateVaultDetails()later treats that as “emergency access disabled”, letting vault creation skip the mandatory step. Keep the flow blocked until the settings load succeeds or the user can retry.💡 Suggested change
const settings = ref<SettingsDto>(); +const settingsLoadError = ref(false); async function loadDefaultEmergencyAccessSettings() { try { settings.value = await backend.settings.get(); + settingsLoadError.value = false; } catch (error) { console.error('Loading emergency access settings failed:', error); + settings.value = undefined; + settingsLoadError.value = true; } } async function validateVaultDetails() { onCreateError.value = undefined; + if (settingsLoadError.value) { + onCreateError.value = new Error('Emergency access settings could not be loaded.'); + return; + } if (!form.value?.checkValidity()) { onCreateError.value = new FormValidationFailedError(); return; }Also applies to: 411-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CreateVault.vue` around lines 344 - 349, The loadDefaultEmergencyAccessSettings function currently swallows backend.settings.get() failures and leaves settings.value undefined (which validateVaultDetails treats as “disabled”), so change the catch to block progress: either rethrow the caught error after logging (so callers must handle/fail the create flow) or set an explicit error flag/state (e.g., settingsLoadError = true and settings.value = null) that validateVaultDetails checks and refuses to proceed; apply the same change to the other settings-load block (the duplicate around the 411–424 area) and update validateVaultDetails to treat an error/unknown settings state as blocking the vault creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue`:
- Around line 115-116: Reset the visible error state at the start of the user
action handlers so old errors don't persist: clear onAddCouncilMemberError.value
at the top of the dialog submit/retry handlers (the function that calls
splitRecoveryKey()) and also at the start of splitRecoveryKey() itself, so each
attempt begins with a clean error state; locate references to
onAddCouncilMemberError, open, and splitRecoveryKey to add the clear before any
async calls or early returns.
---
Outside diff comments:
In `@frontend/src/components/CreateVault.vue`:
- Around line 109-140: The submit handler validateVaultEmergencyAccess() must
mirror the disabled-button guard by checking the EmergencyAccessSetup ref for
existence and for hasValidationErrors and return early if validation fails;
update validateVaultEmergencyAccess() to read this.$refs.emergencyAccessSetup
(or the Composition ref emergencyAccessSetup) and if falsy or
emergencyAccessSetup.hasValidationErrors is true, do not call the child API
(e.g., split()) or proceed with processing. Ensure the early-return is placed
before any invocation of EmergencyAccessSetup methods (like split()) so
keyboard/programmatic submits cannot bypass validation.
---
Duplicate comments:
In `@frontend/src/components/CreateVault.vue`:
- Around line 344-349: The loadDefaultEmergencyAccessSettings function currently
swallows backend.settings.get() failures and leaves settings.value undefined
(which validateVaultDetails treats as “disabled”), so change the catch to block
progress: either rethrow the caught error after logging (so callers must
handle/fail the create flow) or set an explicit error flag/state (e.g.,
settingsLoadError = true and settings.value = null) that validateVaultDetails
checks and refuses to proceed; apply the same change to the other settings-load
block (the duplicate around the 411–424 area) and update validateVaultDetails to
treat an error/unknown settings state as blocking the vault creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44dfdf12-0d06-48ca-a39d-4d6edf5eeef8
📒 Files selected for processing (4)
frontend/src/components/CreateVault.vuefrontend/src/components/emergencyaccess/EmergencyAccessSetup.vuefrontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vuefrontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue
- frontend/src/components/emergencyaccess/EmergencyScenarioVisualization.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue (1)
123-146:⚠️ Potential issue | 🟡 MinorClear error state at the start of
splitRecoveryKey().The error from a previous attempt persists until the next error or dialog close. Clear
onAddCouncilMemberErrorat the start so users don't see stale errors during retry.🔧 Proposed fix
async function splitRecoveryKey() { + onAddCouncilMemberError.value = undefined; if (!emergencyAccessSetup.value) { console.warn('EmergencyAccessSetup not yet mounted.'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue` around lines 123 - 146, The splitRecoveryKey function can surface a stale error from a previous attempt; clear the error state at the start of splitRecoveryKey by resetting onAddCouncilMemberError.value = null (or undefined) before any async work. Update the splitRecoveryKey function (referencing emergencyAccessSetup, props.vault, backend.vaults.createOrUpdateVault, and emit('updated', ...)) to clear onAddCouncilMemberError immediately after the emergencyAccessSetup check and before calling emergencyAccessSetup.value.split so retries don't show stale errors.frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue (1)
741-750:⚠️ Potential issue | 🟡 MinorInsufficient council members falls through to wrong error.
If
councilMembers.value.length < process.details.newRequiredKeySharesfor aCOUNCIL_CHANGEprocess, the condition at line 741 is false, causing fallthrough to line 773's generic "unsupported process type" error. This makes diagnosing completion failures difficult.🔧 Proposed fix
- if (process.type === 'COUNCIL_CHANGE' && councilMembers.value.length >= process.details.newRequiredKeyShares) { + if (process.type === 'COUNCIL_CHANGE') { + if (councilMembers.value.length < process.details.newRequiredKeyShares) { + throw new Error(t('emergencyAccessDialog.error.insufficientCouncilMembers', [councilMembers.value.length, process.details.newRequiredKeyShares])); + } const keyShares = await EmergencyAccess.split(recoveredKeyBytes, process.details.newRequiredKeyShares, ...councilMembers.value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue` around lines 741 - 750, The code path for handling process.type === 'COUNCIL_CHANGE' currently only executes when councilMembers.value.length >= process.details.newRequiredKeyShares, so when there are too few councilMembers it falls through to the generic "unsupported process type" error; add an explicit guard that detects councilMembers.value.length < process.details.newRequiredKeyShares when process.type === 'COUNCIL_CHANGE' and surface a clear error (throw or return a rejected Promise / set a specific error state) before the fallthrough. Locate the block referencing process.type, councilMembers.value, process.details.newRequiredKeyShares, EmergencyAccess.split and backend.vaults.createOrUpdateVault in EmergencyAccessDialog.vue and implement the early check to produce a descriptive error message (e.g., "insufficient council members for council change: need X, have Y") instead of letting execution continue to the unsupported-type branch.
🧹 Nitpick comments (1)
frontend/src/components/CreateVault.vue (1)
462-468: Nullish coalescing is redundant but harmless.The refs
requiredEmergencyKeySharesandemergencyKeySharesare already initialized with0and{}respectively (lines 330-331), so the?? 0and?? {}fallbacks will never be triggered. Consider removing them for clarity, but this is not a functional issue.♻️ Optional cleanup
await backend.vaults.createOrUpdateVault( vaultId, vaultName.value, false, - requiredEmergencyKeyShares.value ?? 0, - emergencyKeyShares.value ?? {}, + requiredEmergencyKeyShares.value, + emergencyKeyShares.value, vaultDescription.value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CreateVault.vue` around lines 462 - 468, The call to backend.vaults.createOrUpdateVault uses redundant nullish-coalescing on refs that are already initialized; remove the "?? 0" after requiredEmergencyKeyShares.value and the "?? {}" after emergencyKeyShares.value and pass requiredEmergencyKeyShares.value and emergencyKeyShares.value directly (keeping vaultId, vaultName.value, false, and vaultDescription.value as-is) so the call uses the existing initialized defaults without extra fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue`:
- Around line 445-450: The code in EmergencyAccessDialog.vue reads
emergencyAccessSetup.value.requiredKeyShares (used to compute
sameRequiredKeyShares) but that property isn't exposed from the
EmergencyAccessSetup component, so it will be undefined; update
EmergencyAccessSetup.vue to expose the requiredKeyShares (via defineExpose or
computed prop) so emergencyAccessSetup.value.requiredKeyShares is available at
runtime and then the sameRequiredKeyShares comparison in the
processType==='COUNCIL_CHANGE' branch will work correctly.
In `@frontend/src/components/emergencyaccess/EmergencyAccessSetup.vue`:
- Around line 110-114: The parent component (EmergencyAccessDialog.vue) tries to
read emergencyAccessSetup.value.requiredKeyShares but requiredKeyShares is not
included in the component's defineExpose call; update the defineExpose call in
EmergencyAccessSetup.vue to include requiredKeyShares (alongside split,
hasValidationErrors, emergencyCouncilMembers) so the ref exposes that property
to the parent, ensuring EmergencyAccessDialog can access
emergencyAccessSetup.value.requiredKeyShares.
---
Duplicate comments:
In `@frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue`:
- Around line 741-750: The code path for handling process.type ===
'COUNCIL_CHANGE' currently only executes when councilMembers.value.length >=
process.details.newRequiredKeyShares, so when there are too few councilMembers
it falls through to the generic "unsupported process type" error; add an
explicit guard that detects councilMembers.value.length <
process.details.newRequiredKeyShares when process.type === 'COUNCIL_CHANGE' and
surface a clear error (throw or return a rejected Promise / set a specific error
state) before the fallthrough. Locate the block referencing process.type,
councilMembers.value, process.details.newRequiredKeyShares,
EmergencyAccess.split and backend.vaults.createOrUpdateVault in
EmergencyAccessDialog.vue and implement the early check to produce a descriptive
error message (e.g., "insufficient council members for council change: need X,
have Y") instead of letting execution continue to the unsupported-type branch.
In `@frontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue`:
- Around line 123-146: The splitRecoveryKey function can surface a stale error
from a previous attempt; clear the error state at the start of splitRecoveryKey
by resetting onAddCouncilMemberError.value = null (or undefined) before any
async work. Update the splitRecoveryKey function (referencing
emergencyAccessSetup, props.vault, backend.vaults.createOrUpdateVault, and
emit('updated', ...)) to clear onAddCouncilMemberError immediately after the
emergencyAccessSetup check and before calling emergencyAccessSetup.value.split
so retries don't show stale errors.
---
Nitpick comments:
In `@frontend/src/components/CreateVault.vue`:
- Around line 462-468: The call to backend.vaults.createOrUpdateVault uses
redundant nullish-coalescing on refs that are already initialized; remove the
"?? 0" after requiredEmergencyKeyShares.value and the "?? {}" after
emergencyKeyShares.value and pass requiredEmergencyKeyShares.value and
emergencyKeyShares.value directly (keeping vaultId, vaultName.value, false, and
vaultDescription.value as-is) so the call uses the existing initialized defaults
without extra fallbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67b2df51-8017-43e5-947d-ba66ded25c7b
📒 Files selected for processing (5)
frontend/src/components/CreateVault.vuefrontend/src/components/emergencyaccess/EmergencyAccessDialog.vuefrontend/src/components/emergencyaccess/EmergencyAccessSetup.vuefrontend/src/components/emergencyaccess/EmergencyAccessVaultList.vuefrontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue`:
- Line 148: The readonly EmergencyAccess summary is binding the current quorum
via recoveryProcess.requiredKeyShares but for COUNCIL_CHANGE it should show the
future quorum from recoveryProcess.details.newRequiredKeyShares; update the prop
passed to EmergencyAccessSetup (the :required-key-shares binding) to use
recoveryProcess.details.newRequiredKeyShares when recoveryProcess.type ===
'COUNCIL_CHANGE' (fall back to recoveryProcess.requiredKeyShares otherwise) so
the dialog shows the new council quorum; locate the EmergencyAccessSetup usage
and recoveryProcess handling to implement this conditional prop change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 914d44f0-2ca6-443b-b540-f0bbc50261e5
📒 Files selected for processing (2)
frontend/src/components/emergencyaccess/EmergencyAccessDialog.vuefrontend/src/components/emergencyaccess/GrantEmergencyAccessDialog.vue
frontend/src/components/emergencyaccess/EmergencyAccessDialog.vue
Outdated
Show resolved
Hide resolved
Contributor
|
I noticed that the TrustDetails popover also inherits the opacity-60, which makes the popover itself semi-transparent and hard to read (see screenshot). @overheadhunter I’ve already found a simple fix for this and would like to commit it, if that’s okay. 
|
…ing TrustDetails popover opacity
Contributor
|
When starting a change council emergency access process, the "no redundancy" badge is currently shown even though the process cannot actually be started. This badge should only be displayed if the defaults in the admin settings allow councils with no redundancy. My suggestion is to only show the "no redundancy" badge when the process is actually startable. |
Contributor
The "New Council Members" field in the "Create Vault" flow remains empty, even when default council members are defined in the admin settings.
|
Member
Author
mindmonk
approved these changes
Apr 1, 2026
Contributor
mindmonk
left a comment
mindmonk
left a comment
There was a problem hiding this comment.
approve for functional testing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the emergency access setup flow in the vault creation and management UI. The main change is the extraction and reuse of the new
EmergencyAccessSetupcomponent, which centralizes logic and UI for configuring emergency council members and required key shares. This simplifies the parent components, reduces code duplication, and improves maintainability. Several related UI and logic improvements are also included.Key changes:
Emergency Access Setup Refactor:
Replaced inline emergency access configuration logic and UI in
CreateVault.vueandEmergencyAccessDialog.vuewith the newEmergencyAccessSetupcomponent. This component now handles council member selection, key share requirements, and related validation, streamlining the parent components. [1] [2] [3] [4] [5]Updated emergency access validation and key splitting logic in
CreateVault.vueto delegate to theEmergencyAccessSetupcomponent, removing redundant state and methods. [1] [2]UI and Usability Improvements:
Improved disabled and readonly states for emergency council member selection, including visual cues (opacity, cursor) in
MultiUserSelectInputGroup.vue. [1] [2] [3]Cleaned up imports and removed unused code from affected components for clarity and maintainability. [1] [2] [3]
Bug Fixes and Minor Enhancements:
Fixed a potential null state issue by ensuring the emergency access step only renders when
vaultKeysare present inCreateVault.vue.Updated button disabling logic to use validation state from
EmergencyAccessSetup, ensuring users cannot proceed with invalid emergency access settings.Other:
MultiUserSelectInputGroup.vue. [1] [2]These changes collectively improve the modularity, reusability, and user experience of the emergency access configuration flow.