fix(secrets): move-dialog reachability, immutable type, and folder-filter refreshes - #600
Open
remko48 wants to merge 9 commits into
Open
fix(secrets): move-dialog reachability, immutable type, and folder-filter refreshes#600remko48 wants to merge 9 commits into
remko48 wants to merge 9 commits into
Conversation
The sidebar's ... menu called deleteSecret() straight from the click handler: the only irreversible action in the app was also the only one with no confirmation, on the path people actually use, because list-view rows carry no ... menu of their own. The table-view row menu already confirmed. Adds SecretDeleteConfirmDialog, registered as the `secret-delete` modal and opened through cnOpenModal like the sidebar's edit, move and share dialogs. It mirrors FolderDeleteConfirmDialog: it owns the delete call and emits `deleted`, so a refused delete (403 on a delegated secret, offline write) keeps the dialog open with the reason inline rather than closing the sidebar behind it, which reads exactly like a delete that worked. Reuses the existing "Delete secret", "Cancel" and "Failed to delete secret" keys; the one new warning string mirrors BulkDeleteDialog's no-trash wording and ships with its Dutch translation. Note this consumes the last unit of slack in the l10n parity ratchet: every locale except nl now sits exactly at its bound, so the next new source string needs translating everywhere.
The edit dialog offered a free Type select, and the payload shape is type-dependent: the card and identity composites render conditionally on `isCard` / `isIdentity` and serialize into `key` differently. Switching a Login to a Card therefore left the card fields empty and the login fields unreachable, and the diff sent the new `typeId` anyway — a saved secret whose declared type no longer matched its stored payload. Bitwarden and 1Password both freeze type after creation for the same reason. Type now renders as read-only text here; the CREATE dialog keeps its select, where there is no existing payload to invalidate. The `typeId` branch is removed from the diff entirely rather than guarded, so even if something later sets `typeId` behind the template's back, this dialog cannot ship a type switch built for the old shape. NcSelect is no longer imported at all. The label reuses the existing `Type` key and the type names already in the catalogue, so no new source strings.
Moving a secret out of a folder made every other secret in the vault appear in that folder. The list passed `folderId` / `typeId` / `search` as call arguments on every fetch and nothing ever wrote them to `filters`, so they stayed permanently null — while the refreshes that run after a mutation call `fetchSecrets()` with NO arguments (the detail sidebar's after edit/move/delete, the vault-contents move's afterwards). Every one of them therefore re-queried the whole vault and replaced the folder's contents with it. The sidebar's own comment asserted the opposite: "fetchSecrets() with no options reuses the store's active filters/sort/page". This is what makes that true. `setListQuery()` records what the list is showing and the list declares it in `reload()` and `goToPage()`. Two things fell out of that: - Paging a type-filtered list silently un-filtered it: `goToPage()` never passed `typeId`. - `fetchAllSecrets()` must NOT inherit the stored query, or an export would shrink to whichever folder was being browsed. It passes WHOLE_VAULT explicitly, and the option lookups test key PRESENCE rather than nullishness so an explicit null means "no filter" instead of falling back to the stored value — which also makes returning to the vault root work.
Moving a secret and moving a vault's contents were two dialogs for one verb, and they had drifted: the vault one drew each candidate with its own icon, colour and tinted circle exactly as the nav rail does, the secret one showed a bare list of path strings. Same choice, different control depending on how you got there — and fixes reached one and not the other. MoveDialog replaces both. `subject` switches the title, the destination label, the picker's candidate set and the vault-only chrome (the emptiness hint, the list of items that stayed behind); nothing else. The two mechanisms stay separate methods behind a two-line dispatch, because one atomic PUT and a non-atomic client-driven loop share a frame and a picker, not a mechanism. `moveVaultContents()` keeps the server-sourced subfolder set and the per-item failure accounting verbatim — that behaviour is what makes a non-atomic move survivable. DestinationSelect is the shared control, and it settles three things the callers kept getting differently: - Options are TREE-ordered: a vault, then its own folders indented beneath it, then the next vault. The flat store order listed every vault and then every folder, so a folder sat pages away from the vault it belongs to and the list read as two unrelated groups. Path labels go with it — the indentation says where a row sits, and a repeated path on every row of a deep tree is noise. A deep tree means a long list; that is the honest trade. - There is NO vault-root destination. A secret always lives in a vault, and a rootless secret has nowhere to be displayed. - `excludeId` drops a whole subtree, so a vault is never offered as a destination for its own contents. The option list teleports again, which also fixes the clipped-to-a-sliver dropdown that made "move a secret" unusable. It had been forced inline because an older @nextcloud/vue painted a teleported list BEHIND NcDialog, leaving the control dead — that evidence predates 9.11.0, which positions the menu with floating-ui and renders it correctly over the dialog. Inline, it was instead clipped by the dialog's 52px content box, and reserving room for it made every move dialog 300px tall. The history is preserved in the component so nobody has to rediscover it. The e2e workflow gains a geometric assertion, because the spec already clicked that option and passed throughout the bug: Playwright scrolls an element into view before clicking, so the test that covered the flow was structurally blind to a control no human could reach. It now measures the list against the viewport and any clipping ancestor. The vault-contents spec moves to MoveDialog.vaultContents intact, and the secret path gains the component test SecretMoveDialog never had — including that it never runs the vault machinery, which is the risk a merged dialog carries. Every label reuses an existing catalogue key; no new source strings.
BulkMoveDialog held a third copy of the "vault root plus every folder, path-labelled" builder, with no icons and without turning off the teleport, so the same choice looked different again and this one still had the dead-control variant on older library versions. It now renders DestinationSelect, which also removes a latent trap: the Move button keyed off `!!targetFolder`, which worked only because the old select bound whole option objects. With a reduced value and a vault-root option, null would have been indistinguishable from "nothing picked" and bulk-move to the vault root would have been impossible. There is no root destination any more, so null and unchosen are the same state and the simple check is correct.
Contributor
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-09-02 12:42 UTC
Download the full PDF report from the workflow artifacts.
Three CI checks were red on this branch and none of them should have reached CI: - `lint-check` and `Vue Quality (eslint)`: one error, `perfectionist/ sort-imports` in src/registry.js. Swapping SecretMoveDialog's import for MoveDialog in place left the list out of alphabetical order. - `Hydra Gates`: gate-16 spec-coverage, four changed methods with no `@spec` annotation — BulkMoveDialog::moveOne, secret store::fetchAllSecrets, and SecretList::reload / ::goToPage. Each gets the anchor its neighbours already use: the bulk-operations requirement for the per-item move, and list-and-pagination for the three list/paging methods. Also declares SecretEditDialog's `emits: ['saved', 'close']`. It emitted both without declaring either, which `vue/require-explicit-emits` warned about in a file this branch already touches. Verified locally against the same gate runner CI uses, rather than by pushing and waiting: eslint 0 errors, stylelint clean, hydra exit 0 over 72 gates, prettier clean, 744 unit tests green.
Contributor
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-02 13:20 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 536/536 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-03 07:21 UTC
Download the full PDF report from the workflow artifacts.
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
1 participant
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.
Depends on
#599 must be merged first. This branch is cut from
fix/must-list-bug-sweep, so until #599 lands its commit also shows in thisdiff; retargeting this PR's base at that branch shows the four commits below
on their own.
fix(secrets): freeze a secret's type after creation
The edit dialog offered a free Type select while the payload shape is
type-dependent — the card and identity composites render conditionally on
isCard/isIdentityand serialize intokeydifferently. Switching a Loginto a Card left the card fields empty and the login fields unreachable, and the
diff sent the new
typeIdanyway, so a saved secret's declared type no longermatched its stored payload. Bitwarden and 1Password both freeze type after
creation for the same reason.
Type is read-only text here; the CREATE dialog keeps its select, where there is
no existing payload to invalidate. The
typeIdbranch is removed from the diffrather than guarded, so a later change that sets
typeIdbehind the template'sback still cannot ship a type switch built for the old shape.
fix(secrets): keep the folder filter when the list refreshes
Moving a secret out of a folder made every other secret in the vault appear in
that folder.
The list passed
folderId/typeId/searchas call arguments on everyfetch and nothing ever wrote them to
filters, so they stayed permanentlynull — while the refreshes that run after a mutation call
fetchSecrets()withno arguments. Each therefore re-queried the whole vault and replaced the
folder's contents with it. The sidebar's own comment asserted the opposite
("reuses the store's active filters/sort/page"); this makes that true.
Two things fell out: paging a type-filtered list silently un-filtered it
(
goToPage()never passedtypeId), andfetchAllSecrets()must NOT inheritthe stored query or an export would shrink to the browsed folder — so it passes
a whole-vault scope explicitly, and the lookups test key presence rather than
nullishness, which also makes an explicit "no filter" work.
refactor(dialogs): one move dialog and one destination picker
Moving a secret and moving a vault's contents were two dialogs for one verb,
drifted apart: one drew each candidate with its own icon, colour and tinted
circle as the nav rail does, the other showed bare path strings — and fixes
reached one and not the other.
MoveDialogreplaces both;subjectswitchesthe title, the destination label, the candidate set and the vault-only chrome,
and nothing else. The two mechanisms stay separate methods behind a two-line
dispatch, because one atomic PUT and a non-atomic client-driven loop share a
frame and a picker, not a mechanism.
DestinationSelectis the shared control and settles three things:The flat store order listed every vault and then every folder, so a folder
sat pages from the vault it belongs to. A deep tree means a long list; that
is the honest trade.
rootless secret has nowhere to be displayed.
excludeIddrops a whole subtree, so a vault is never a destination for itsown contents.
The list teleports again, which fixes the clipped-to-a-sliver dropdown that
made "move a secret" unusable. It had been forced inline because an older
@nextcloud/vue painted a teleported list behind NcDialog, leaving the control
dead; that evidence predates 9.11.0, which positions the menu with floating-ui
and renders it over the dialog correctly. The history is kept in the component.
The e2e workflow gains a geometric assertion, because the spec already clicked
that option and passed throughout the bug: Playwright scrolls an element into
view before clicking, so the test covering the flow was structurally blind to a
control no human could reach.
The vault-contents spec moves across intact — it pins the server-sourced
subfolder set and the per-item failure accounting — and the secret path gains
the component test it never had, including that it never runs the vault
machinery, which is the risk a merged dialog carries.
refactor(bulk): move the bulk-move target onto the shared picker
BulkMoveDialogheld a third copy of the same option builder, without icons andwithout the teleport fix. Rewiring it also removed a latent trap: its Move
button keyed off
!!targetFolder, which worked only because the old selectbound whole option objects — with a reduced value and a vault-root option, null
would have been indistinguishable from "nothing picked" and bulk-move to the
root impossible.
Note for review
No new l10n strings in this branch: every label reuses an existing catalogue
key, so the parity ratchet is untouched.