feat(vault): Proton-style vault icons and colors, vault actions, and post-#479 fixes - #591
Merged
Merged
Conversation
Clicking a row now opens the secret in an NcAppSidebar over the vault list instead of navigating to a separate page, so the list context (folder, search, pagination) stays visible and intact behind it. The detail page is gone: the id rides as an optional segment on the list routes (/secrets/:id? and /folders/:folderId/:id?), because CnPageRenderer keys its render on the page id and a separate page remounted the list on every open. Deep links to #/secrets/<id> keep resolving (list + open sidebar) and stay behind the vault-lock guard. Route <-> sidebar mapping lives in src/utils/detailRoute.js. The sidebar lays the detail out Proton Pass style: the action row (labeled Edit opening the edit dialog, icon-only Share with ariaLabel, and a "Secret actions" menu holding Move, Delete and Close - the native X is hidden while the row renders; Esc still closes everywhere) sits with the title in the header; fields render as grouped icon/label/value boxes with a visible attachments box and a last-modified/created metadata box (NcDateTime); the informational tail folds into "More information" (sharing, requests, activity) and owner-only "Advanced" (versions, rotation, honey) disclosures. Card and identity secrets render their payloads as first-class rows (masked number/CVV/PIN/BSN with eye + copy, revealed numbers regrouped in blocks of four, identity split into Personal/Address/Contact sections); the old CardDisplay/IdentityDisplay components are deleted and the raw key row is hidden for composite types. The pane widens to clamp(300px, 35vw, 560px) above the library's mobile breakpoint. Decryption semantics are unchanged. Edit/Move refresh the list behind the sidebar; Delete closes it. The SecretDetail vitest specs are retargeted at the sidebar component; the new route mapping has its own spec; e2e drives Move/Share through the new action row and opens the More-information disclosure for the activity assertion; the guards spec drops the removed page id.
New source strings from the sidebar restyle - "Details", "Versions", "Secret actions", "More information", and the identity section headings "Personal details" / "Address details" / "Contact details" - translated into English plus all 36 required locales, with the .js catalogs regenerated and the parity ratchet tightened to record the progress.
Opening the folder the "..." stand-in node represents lit TWO rows in the rail: the node itself (vue-router exact-active styling on its link) and its parent, which received the deepest-visible-ancestor highlight. The ancestor rule now returns nothing when the active folder is the single hidden child the "..." stands for, so exactly one row reads as selected. Navigation through the node is unchanged, and the multi-hidden-children case keeps highlighting the parent row as before.
Navigating to another folder used to keep the previous folder's rows on screen until the new ones landed, then swap them mid-air - CnIndexPage only shows its full loading spinner while it is loading AND has nothing to render. A folderSwitching flag now blanks the list for the duration of the navigation's fetch, so a switch reads as spinner -> new folder's contents. The flag starts true because root <-> folder navigations REMOUNT the view (the routes are different manifest pages and CnPageRenderer keys its render on the page id), and a fresh mount rendered the store's previous rows before mounted() fetched anything - the folderId watcher only covers folder -> folder moves. In-folder refreshes and searches keep the in-place behavior.
A sweep over every locale for values identical to their English source
found six bulk-copied strings: "Applications awaiting approval", "No
applications awaiting approval" (31 locales), "Your secrets, folders
and applications at a glance", "Vault overview", "Rotation due" (33
each) and "View all" (31). All now carry real translations phrased
with each locale's established terminology. Script-mismatched
loanwords are also fixed: Token and Status become Cyrillic in mk/sr,
and Link becomes Länk/Ligação/Colliaziun in sv/pt/rm. Values that
already held a genuine translation were never touched, and legitimate
loanwords (URL, Token in Latin-script locales, Dashboard, Widgets,
Layout) and the brand string "Keepiq {version}" stay as they are. The
.js catalogs are regenerated.
The secret types come from the server with English labels seeded by SeedSecretTypes.php, and the frontend printed type.label raw - so the type filter, the create and edit dialogs' type selectors and the detail sidebar's type subline showed English in every language. A new secretTypeLabel() helper (src/utils/secretTypes.js) routes the label through the translator at all four sites, falling through unchanged for custom types without a catalog entry - the dynamic-key pattern the nav already uses for manifest menu labels. The ten system labels are translated in English plus all 36 required locales (295 new entries; seven keys were missing from every catalog), the .js catalogs are regenerated and the parity ratchet is tightened accordingly.
… l10n commit Commit 17988a7 accidentally carried a local development aid into LockScreen.vue: a DEV-ONLY auto-unlock block that reads the master password in plaintext from localStorage and unlocks the vault on page load, plus its status banner and data field. It was guarded by a NODE_ENV check (webpack strips it from production builds) and inert without an explicit localStorage opt-in, but it defeats the lock screen's security model and was never meant to be tracked. The same commit also dropped two @SPEC tags from handlePasskeyUnlock; they are restored. The file now matches its pre-17988a77 state exactly.
…p-row toolbar The vault list adopts CnIndexPage's own selection: selectable with rowClickToView (clicking opens the detail, checkboxes select), selectedIds fed from the bulk store and @select synced back. The table gets header select-all, row checkboxes and selected-row highlight; cards get a checkbox each; the list keeps shift-range selection through NcCheckboxRadioSwitch — the same Nextcloud checkbox the other views render — with a capture-phase shift recorder. One selection is shared across view switches, pruned to the visible rows on every list change (folder navigation, page flips, filters, refresh), and the strip's bulk buttons disable while the list is (re)loading so a stale selection from the previous page cannot be moved or deleted. Bulk actions live in the library's contextual selection strip alone (live count announced via role="status" — WCAG 2.1 SC 4.1.3 — plus Move, Share, Add to team folder and Delete with icons, and the strip's own Clear; bulk-actions §3.1's required bar). The Actions menu keeps only the Select-all checkbox — the sole whole-view select-all for the list and card views. Selected list rows tint the item with its hover token, never the checkbox gutter. Folders left the collection: vaults/subfolders render as a captioned strip (Vaults at root / Folders inside) above the collection in every view mode (the library's #before-collection slot), so they no longer masquerade as secrets in table/cards and survive pagination; the strip owns the single divider. Breadcrumbs render below the actions bar, above that strip, a step larger — the trail is the folder heading now that the page title is visually hidden (the h1 stays for accessibility). The bar owns the top row: keepiq's doubled page padding is dropped and the bar clears the floating nav toggle with a 30px !important margin — a margin so its background starts after the toggle, !important because design-system themes flatten the bar's box with their own !important rules. Keepiq-scoped, as is the container-scale rounding; other apps keep their heading and the library defaults. The secret-type filter is a funnel button beside the search field carrying the sort radios too; it flips to its filled glyph in the primary color whenever a type filter is set or the sort deviates from the default name sort. New l10n string "Filter and sort" in en + all 36 required locales. e2e: clickOverflowAction opens the bar menu via a new openActionsMenu helper; export-gdpr's four inline menu openers use it.
… display cap Being in a folder deeper than the rail renders (e.g. test11 under the 5-level cap) highlighted the deepest visible ancestor (test5), which read as the wrong folder being open. The active trail is computed once (activeFolderTrail); when it exceeds the cap no ancestor row lights up and the "…" node under the deepest visible branch renders active instead (ellipsisHighlightId, passed into NavFolderTree's ellipsis item) — exactly one row selected, and it is the row that stands for the hidden chain. This subsumes the earlier single-hidden-child special case.
ImageMagick's internal SVG renderer — used by Nextcloud theming to generate the favicon — does not draw stroked paths, so the shackle vanished from generated favicons. All five icons (app, app-dark, app-store, pwa-icon, pwa-icon-maskable) redraw the shackle as a filled outline of equivalent thickness, so stroke-less rasterizers render the full lock.
CI's stylelint (declaration-empty-line-before) rejects a blank line between a custom property and the following declaration — a formatting habit that is valid in nextcloud-vue's stylelint config but not in keepiq's. The full stylelint sweep is green again, and RESTYLE-PLAN's per-stage verification list now names stylelint so the gate stops being skippable locally.
gate-16: @SPEC tags on the seven changed methods the gate named — onRowCheck/onSelectAll (bulk-actions multi-select requirement), the selectedFolderId watcher (folder management), typeFilterOptions and both secret dialogs' typeOptions (secret types), and the detail sidebar's mounted() (read secret). gate-40: the list checkbox's ariaLabel binding becomes aria-label — identical Vue behaviour, and the form-label rule recognises the kebab form. gate-45: the sidebar's accordion-chevron transition gains the prefers-reduced-motion fallback the rest of the file already has. gate-46: five @SPEC tags pointed at anchors that never existed — requirement-secret-sharing is requirement-share-a-secret (user-sharing) and requirement-cached-vault-is-read-only is requirement-offline-mode-is-strictly-read-only (offline cache).
Brings the released library features the vault page consumes, until now only available through a locally built dist overlaid into node_modules: the contextual selection strip (live role="status" count, #selection-actions slot, clear-selection wiring), the #after-search and #before-collection CnIndexPage slots, the role-grouped actions-bar layout with its narrow-width container-query tiers, clickToView on CnCardGrid/CnObjectCard, and the CnPageRenderer fix that stops lifted manifest fields (title, icon) falling through as HTML attributes on custom pages — the "Vault" tooltip that hovered over every custom page. With the package live, the overlay workflow is retired for this line of work: node_modules now matches the lockfile again.
…yword CI's stylelint (declaration-property-value-keyword-no-deprecated, in the newer toolchain the merged lockfile installs) rejects `word-break: break-word` — a deprecated keyword that always aliased overflow-wrap behaviour. The detail rows' value wrapping now uses `overflow-wrap: anywhere`, which wraps long secret values identically.
Four review conversations on the merged PR #479, all in SecretDetailSidebar: - isOwner reads the single canonical `ownerId` field; the owner_id/userId fallback chain was dead code (the Secret entity has serialized ownerId since its first version) and the pinning test now asserts fail-closed behaviour for legacy-style payloads instead. - mounted() catches a failing fetchTypes() and surfaces the error instead of stranding the sidebar on an infinite spinner. - refreshList() no longer swallows a failed list refresh silently; it shows an error toast so a stale list is signalled. - remove() catches a refused delete (403/500/offline) and shows the reason inline instead of leaving the sidebar open without feedback.
The secret detail sidebar swaps the route's :id segment without remounting, so AttachmentPanel's mounted()-only fetch kept showing the first opened secret's attachments on every secret opened after it. Add a secretId watcher that resets the attachment store (no stale rows while loading, none left behind on a failed fetch) and refetches, mirroring the sidebar's own route-driven reload. Covered by a new component regression test.
The manifest's first-visit tour was translated only in Dutch; the other 36 catalogues missed 12 of its 13 strings and the three menu labels it points at (Features & roadmap, My activity, Flows). Add translations to every catalogue, embedding each language's own menu-label wording so the tour text matches the rendered nav — the Dutch tour is fixed on the same point (it quoted the English labels). Correct wrong-language entries for the referenced labels (Czech/Slovak/Slovenian/Macedonian carried Croatian; Belarusian/Ukrainian carried Russian; Catalan Spanish; Swedish Danish; Luxembourgish German; Romansh Italian), add entries for the two strings introduced by the PR #479 sidebar fixes, and regenerate the .js catalogues (npm run l10n:build).
Restyle Stage 9 backend: nullable custom_icon + custom_color KEY columns (lowercase kebab, max 64 chars — never hex or free text; the frontend owns the catalogs, and an unknown key renders as the fallback glyph there, which keeps old servers forward-compatible with newer frontends) on doriath_folders, serialized on the Folder entity so the offline vault cache carries them for free. FolderService gains updateAttributes(id, changes, userId): owner-checked, key-present-with-null CLEARS, absent key untouched — so a rename/move request that does not mention the attributes can never wipe a color (the 53a3600 semantics). create() forwards the two optional keys through FolderTreeService; both paths validate the key format. The controller applies attribute changes only for keys PRESENT in the raw request body (array_key_exists on getParams()), the distinction the typed arguments cannot make. Covered by seven new FolderServiceTest cases: set / clear / untouched, ownership rejection, format rejection on both paths, create-with-keys.
…ult dialogs Restyle Stage 9 dialogs, over the library's new CnIconColorPicker (color swatches + searchable icon grid + themed preview; the Default cells emit explicit null, which is the whole reset story): - FolderCreateDialog: the picker at VAULT level, and the create flows are now FIXED by the opening context (team decision) — a vault is only ever created at the root, so the New-vault dialog has no parent picker at all, and a folder only ever inside a vault, so the folder flow's parent picker no longer offers the root. The two flows can no longer morph into each other mid-dialog. - NEW FolderEditDialog: rename + (vault-level) the picker, seeded from the folder. This REINTRODUCES rename — the store's updateFolder had no UI caller since the old FolderTree left. For a vault the two customization keys are always sent, so a Default pick genuinely clears server-side (key-present-with-null). - NEW FolderMoveDialog: vaults themselves never re-parent (team decision) — "Move vault contents" transfers everything INSIDE the vault into another vault (direct secrets via updateSecret folderId- only, direct subfolders with their subtrees via updateFolder). The target picker offers only the OTHER vaults, each rendered with its own icon and color; an empty vault says there is nothing to move; a mid-way failure is reported honestly (the transfer is item-by-item, no bulk endpoint exists). - Store: createFolder forwards the two keys; updateFolder documents the null-clears contract. The vitest library stub grows the picker and the catalog resolvers. Covered by new FolderCreateDialog and FolderEditDialog component specs (vault/folder flow split, payload shapes, explicit-null reset, inline error).
… a vault actions menu Restyle Stage 9 rendering + the vault "..." menu: - NavFolderTree depth-0 entries render the vault's picked icon in its theme-variant color on the Proton-style tinted circle — the tint is derived from the SAME resolved hex (folderColorTint, the 53a3600 approach), so glyph and circle can never disagree across a live theme flip (currentTheme() is reactive). On the ACTIVE row the circle goes OPAQUE in the theme's main background with the colored glyph on it: exactly the rest-state foreground/background pairing at unchanged contrast, so the color identity survives the selection without the glyph ever sitting on the saturated primary highlight. Every fill falls back to the string 'currentColor' — an explicit null fill-color strips the SVG fill attribute and renders black regardless of theme. - Each vault entry carries an actions menu (vertical-dots trigger via the menu-icon slot; the trigger goes transparent on the active row instead of reading as a stray light pill): Edit vault / Share vault / Move vault contents / Delete vault. KeepiqAppNav hosts the dialogs — share reuses TeamFolderDialog (the same team-sharing flow as the list toolbar), delete finally WIRES UP the orphaned SubfolderResolutionDialog (per-subfolder keep/move/delete protocol) with a plain confirm for empty vaults, and navigates back to the vault root when the open folder was the deleted one. - The list's vault rows (SecretList #before-collection strip) render the same glyph treatment; subfolderRows() passes the two keys through as explicit nulls when unset. Covered by the extended vault-list spec (pseudo-row pass-through).
The Stage-9 catalog and dialog strings in en + all 36 required locales, with the .js browser catalogs regenerated (npm run l10n:build): - the picker labels routed through CnIconColorPicker's translate prop (Color, Icon, Search icons, Default), the 12 color names and the 42 icon names (each language reuses its existing vault/menu terminology); - the vault actions menu and dialogs: Edit/Share/Delete vault, Move vault contents, the contents-move explainer, empty-vault lines, the Target vault label and the honest partial-failure error, plus the empty-delete confirmations; - the two PR #479 sidebar strings that were still missing from the earlier catalog commit (Could not refresh the list, Failed to delete secret). The earlier "Move vault"/"Move folder"/re-parent hint strings are retired but kept in the catalogs, matching how other superseded strings are handled. Extraction and parity gates green.
The keep-the-color-when-selected attempt (an opaque main-background disc under the colored glyph) is rejected on review: the disc read as a stray pill on the row highlight. On the highlighted row the vault glyph now follows the row contrast color — white on the highlight, exactly like the collapse chevron — with no tint circle behind it. Color identity shows at rest and on hover. The flattening is enforced in CSS, keyed to the row's own `.active` class rather than only the highlightId prop: the class has MORE sources than the prop (NcAppNavigationItem also activates through vue-router's own link matching), and a colored vault carries its hex as the svg fill ATTRIBUTE, which only a fill PROPERTY overrides. Two companion rules fix inheritance gaps NC's legacy-active styling leaves open: the icon column gets the primary-contrast token explicitly (the active rule whitens only the LINK element, so inherited currentColor resolved to main-text black on themed rows), and the "..." actions trigger gets the same token (its earlier `color: inherit` fell into the same trap — the collapse chevron is only white because NC hands it the tertiary-on-primary variant, which the menu toggle never gets). Covered by a new NavFolderTree glyph spec (highlighted = currentColor fill + no tint style; rest = color + same-hex tint; collapsible and leaf vaults identical).
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-01 11:31 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
requested changes
Sep 1, 2026
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
Contributor
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (Strict)
Security posture on the FolderService / FolderController side is solid — endpoint-inventory, newly-persisted-field audit (custom_icon / custom_color), catch-block fail-mode audit, and cross-path rule consistency all clean. Owner check fires on every mutation path (create / rename / move / updateAttributes / delete); the raw-body-key-presence check via array_key_exists on getParams() correctly gates "absent-key-untouched vs null-CLEARS" semantics; the 7 new FolderServiceTest cases cover positive AND negative paths (invalid format, foreign owner, absent key, null clears). assertAttributeFormat rejects hex, uppercase, spaces, and free text. The fix(security): remove the dev auto-unlock commit is verified fully clean at HEAD (no residue on any autoUnlock / AUTO_UNLOCK / unlockOnMount / import.meta.env.DEV symbol). Deleted CardDisplay / IdentityDisplay have no orphan callers.
One blocker + four Strict-mode concerns hold the verdict:
- 🔴 Inline
NcDialogin a component outsidesrc/dialogs//src/modals/—KeepiqAppNav.vue:112-160is the sole.vueoutsidesrc/dialogs//src/modals/that rendersNcDialog; extract tosrc/dialogs/DeleteFolderDialog.vue - 🟡
AttachmentPanelsecretIdwatcher: stale-response race +store.reset()doesn't clearloading - 🟡
FolderMoveDialogreads subfolders from a potentially-stale Pinia store — silent-skip risk on subfolders added after the last store hydration - 🟡
FolderMoveDialognon-atomic submit loop, first failure aborts remaining items with no per-item feedback
Additional finding — not anchored inline because target lines sit outside the diff hunk after the development merge pulled in #479: SecretDetailSidebar.vue's secretId watcher (line ~1337) calls load(), and load() does this.secret = await useSecretStore().fetchSecret(this.secretId) with no request-id / AbortController guard — a rapid navigation between secrets can land the response for the previous secretId over the current one's state. #573 (fix(vault): retire a secret's plaintext when the sidebar loads another) added secretLoadToken as a :key on the reveal panel, which mitigates plaintext-leak-across-secrets but does NOT guard the fetchSecret assignment itself. Adding a monotonic loadSeq counter (const seq = ++this._loadSeq; ... if (seq !== this._loadSeq) return) after every await in load() closes the remaining race.
Dependency note: This PR imports CnIconColorPicker, resolveFolderColor, resolveFolderIcon, folderColorTint, currentTheme, and searchFolderIcons from @conduction/nextcloud-vue. Those names are provided by ConductionNL/nextcloud-vue#906 (approved in this review round); the version bump can only land after that merge + release.
Merge status: mergeable=MERGEABLE at HEAD 992b6e68be — the earlier CONFLICTING state was resolved when origin/development was merged in.
CI at HEAD: CodeQL / Analyze (actions/go/js-ts) / Conflict-markers-and-PHP-syntax all green.
Settled after trying both alternatives live (a plain white glyph, and a translucent tint that sank into the highlight): the selected row shows the vault's COLORED glyph on a disc in the theme's main background — a white disc in light mode, a dark disc in dark mode. That recreates exactly the rest-state foreground/background pairing (light palette variants on a light surface, dark variants on a dark one), so the color identity survives selection at unchanged contrast. Colorless vaults keep no circle and follow the row's text color, and the icon-column CSS rule (which also keeps the chevron side and nested folder glyphs legible on the highlight) stays — it cannot touch colored glyphs, whose hex rides the svg fill attribute. The glyph spec is re-pinned to this contract and records the decision so it is not relitigated.
The release that carries nextcloud-vue#906 — CnIconColorPicker, the folder-customization catalogs and resolvers, folderColorTint and useCurrentTheme — so keepiq builds from a clean install again and the Stage-9 local-dist overlay retires (the Stage-4 CnBreadcrumbs pattern, now closed out the same way). Verified against the published package: all picker polish rounds are in (the release build extracts component CSS to dist/esm/nextcloud-vue.css rather than inlining it), the PR review added keyboard a11y and spacing tokens on top, and the library now imports dexie as a BARE dependency instead of snapshotting a copy into its dist — one Dexie per page by construction, resolved from the app (4.4.5), which retires the two-versions crash class for good.
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-01 13:02 UTC
Download the full PDF report from the workflow artifacts.
Six phpcs errors and one phpmd violation, all introduced by the vault customization work and all blocking PR #591: - FolderController's update() docblock started its long description in lower case, and the migration used an inline `/** @var */` block where the standard wants `/* */`. - Four assertAttributeFormat() calls passed positional arguments; the standard requires named parameters for internal calls. - FolderController::update() reached a cyclomatic complexity of 12 (the threshold is 10 and it trips AT the threshold, not above it) once the attribute branches joined the rename/move ones. Two private helpers now carry them: attributeChanges() collects the key-present-with-null changes from the raw request body, and applyUpdates() runs the rename/move/attributes sequence and resolves the folder when nothing mutated. update() is left with the auth check and the error mapping, which is what it should have been reading as all along.
…alog Hydra gate-13 (modal-isolation) failed on PR #591: the empty-vault delete confirmation was an inline NcDialog inside KeepiqAppNav, and the gate requires modals to live in src/dialogs/ or src/modals/. It is now FolderDeleteConfirmDialog, which owns its own delete call, busy state and inline error and emits `deleted` — the same contract SubfolderResolutionDialog already offers, so the rail hosts both the same way and keeps only the choice between them. Also adds the @SPEC anchors gate-16 (spec-coverage) wants on the rail methods this branch introduced — onDeleted, onMoved and leaveDeletedRoute trace to folder-management, resetDelete is tagged exclude as dialog-host state plumbing.
… frontend The two remaining CI failures on PR #591: - Frontend Check (format): nine files from this branch were never run through prettier. Note for anyone reproducing this on Windows — with core.autocrlf=true every file fails `npm run format`, because the working copy is CRLF and prettier expects LF; run it with `--end-of-line auto` to see the real offenders. - Hydra gate-16 (spec-coverage): the dialogs this branch adds are new files, so every method in them is in the diff and needs an @SPEC anchor. The domain methods trace to folder-management; the dialog open-state plumbing and the form-enablement guards are tagged exclude with the reason at the point of use, which is what the gate's waiver mechanism is for.
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-01 14:22 UTC
Download the full PDF report from the workflow artifacts.
Gate-16 (spec-coverage) reports the watcher this branch adds to AttachmentPanel as a changed method with no @SPEC anchor. The watcher re-runs the panel's existing fetch when the hosting sidebar swaps the :id route segment. It states no attachment behaviour of its own — the six requirements in openspec/specs/encrypted-attachments cover upload, envelope shape, quota, export, deletion cascade and audit, none of them a refetch-on-route-swap — so it carries a reason-bearing @SPEC exclude rather than a false anchor to the upload requirement. The file's header anchors continue to state the behaviour itself.
WilcoLouwerse
previously approved these changes
Sep 1, 2026
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
Contributor
There was a problem hiding this comment.
Verdict: APPROVE (Quick re-review, fix-commit window)
The 🔴 blocker from the prior round is resolved:
- 🔴 Inline
NcDialoginKeepiqAppNav.vue→ extracted tosrc/dialogs/FolderDeleteConfirmDialog.vueine4076e23f2— clean peer ofSubfolderResolutionDialog, host mounts it via the same shape. Grep confirms zeroNcDialog/NcModalmarkup outsidesrc/dialogs//src/modals/.
Also worth calling out from the fix window:
- ✅
chore(deps): take @conduction/nextcloud-vue 2.29.0— the #906 dep bump landed - ✅
FolderController::update()refactored —attributeChanges()+applyUpdates()extracted as private helpers; the raw-body-key-presence semantics are unchanged, just isolated - ✅
NavFolderTree.vuevault highlight rewritten — selected vaults now render their colored glyph on an opaque--color-main-backgrounddisc, so the color identity survives selection; the two!importantCSS rules that flattened the glyph on the active row were removed - ✅ Prettier + phpcs/phpmd +
@specanchors added across the Stage-9 surface
Three 🟡 concerns from my prior review are still open — no fix commit touched these paths. Under Quick mode they don't block the verdict, but they're worth addressing before merge (or in a follow-up PR):
- 🟡
AttachmentPanelsecretIdwatcher — stale-response race +store.reset()doesn't clearloading - 🟡
FolderMoveDialogreads subfolders from a potentially-stale Pinia store - 🟡
FolderMoveDialognon-atomic move loop — first failure aborts remaining items with no per-item feedback
CI heads-up (not blocking — not in required checks, and were already failing at the previous head, but worth fixing before merge):
quality / PHPUnit(×6 matrix) — one failure:SuppressionHygieneTest::testEverySuppressionCarriesAWrittenReasonfails onlib/Migration/Version000035Date20260901000000.php:42— the@SuppressWarnings(PHPMD.UnusedFormalParameter)tag has no written reason (≥12 chars on the tag's own line). Simple fix: append a rationale after the tag, e.g.@SuppressWarnings(PHPMD.UnusedFormalParameter) NC migration signature requires these positional arguments.quality / Quality Report+quality / Hydra Gates— failing; my first-review verdict body wrongly implied all-green, sorry — I checked only CodeQL / Analyze / Conflict-markers. Worth a look before merge.
SuppressionHygieneTest requires every @SuppressWarnings under lib/ to carry its reason on the tag's own line, at least 12 characters after the closing paren. The bare tag added to satisfy phpmd in fc2a9ee met phpmd and failed that test, taking the whole PHPUnit matrix with it. $output and $options are unused because SimpleMigrationStep::changeSchema() mandates them; the reason now says so, in the same shape the sibling migrations and background jobs use.
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-01 16:46 UTC
Download the full PDF report from the workflow artifacts.
…another secret's files The secret-detail sidebar swaps its :id without remounting, so AttachmentPanel refetches in place. Nothing ordered those responses: a slow list for the previous secret could land after the current one and repopulate the panel with a different secret's attachment metadata. fetchAttachments now takes a monotonic listRequestId and writes attachments, error and loading only while that id is still current, and reset() bumps the id so an in-flight request is retired rather than allowed to repopulate after the reset. reset() also clears loading, which it never did — a reset during a fetch left the spinner running with nothing left to finish it. The guard sits in the store because that is where the write happens; a component-level key can only decide whether to start a fetch.
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-01 17:21 UTC
Download the full PDF report from the workflow artifacts.
bbrands02
approved these changes
Sep 2, 2026
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
3 participants
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.
Important
Depends on ConductionNL/nextcloud-vue#906 — the vault dialogs and the rail rendering consume
CnIconColorPicker, the folder-customization catalogs (FOLDER_COLORS/FOLDER_ICONS,resolveFolderColor/resolveFolderIcon/searchFolderIcons/folderColorTint) anduseCurrentTheme/currentThemefrom that PR. It must be merged and released first, followed by a dep bump here; until then this branch builds against a locally overlaid dist.What
Restyle Stage 9 — per-vault icons & colors (Proton pattern)
feat(folders): persist per-folder custom icon and color— nullablecustom_icon/custom_colorKEY columns (lowercase kebab, max 64 chars; the frontend owns the catalogs, unknown keys render as the fallback glyph, keeping old servers forward-compatible) ondoriath_folders, serialized on the entity so the offline cache carries them for free.FolderService.updateAttributes()is owner-checked with key-present-with-null CLEARS / absent-key-untouched semantics, so a rename/move can never accidentally wipe a color; the controller applies attributes only for keys present in the raw request body. Seven new FolderServiceTest cases.feat(vault): per-vault icon and color, Proton-style, picked in the vault dialogs— the picker in FolderCreateDialog (vault level), plus two new dialogs. Create flows are fixed by opening context (team decision): a vault is only ever created at the root (no parent picker at all), a folder only ever inside a vault (no root option) — the flows can no longer morph into each other mid-dialog. FolderEditDialog reintroduces rename (the store'supdateFolderhad no UI caller since the old FolderTree left) and seeds the picker; the Default cells' explicit nulls genuinely clear server-side. FolderMoveDialog implements the team decision that vaults never re-parent: "Move vault contents" transfers everything inside the vault into another vault (secrets via folderId-onlyupdateSecret, subfolders with their subtrees), target picker offers only the other vaults rendered with their own glyphs, empty vaults say there is nothing to move, and mid-way failures are reported honestly (the transfer is item-by-item; no bulk endpoint exists).feat(vault): render vault icons and colors in the rail and list, with a vault actions menu— depth-0 rail entries and the list's vault rows render the picked icon in its theme-variant color on a tinted circle derived from the SAME hex (folderColorTint), reactive to a live light/dark flip. Every vault entry gets a vertical-dots actions menu: Edit / Share (reuses TeamFolderDialog) / Move vault contents / Delete — delete finally wires up the orphaned SubfolderResolutionDialog (per-subfolder keep/move/delete protocol) with a plain confirm for empty vaults, navigating back to the vault root when the open folder was deleted.fix(vault): selected rail entries render plain contrast glyphs— on the highlighted row the glyph flattens to the row's contrast color (white, like the collapse chevron), enforced in CSS keyed to the row's own.activeclass: the class has more sources than the highlight prop (vue-router link matching), and a colored vault's hex lives in the svg fill attribute, which only a fill property overrides. Companion rules fix two inheritance traps in NC's legacy-active styling (icon column and the "⋮" trigger both resolved to black). New NavFolderTree glyph spec.l10n(vault): Stage-9 strings across all locales— picker labels, 12 color + 42 icon names (matching each language's existing vault terminology), the vault-action and dialog strings, plus two PR feat(vault): detail sidebar, role-grouped toolbar and fleet-style selection #479 sidebar strings that had missed the earlier catalog commit. en + all 36 required locales,.jscatalogs regenerated, extraction + parity gates green.Fixes on the merged #479 surface
fix(sidebar): address PR #479 review feedback—isOwnerreads the single canonicalownerId(the fallback chain was dead code; the pinning test now asserts fail-closed for legacy-style payloads),mounted()surfaces a failed type fetch instead of stranding the sidebar on its spinner,refreshList()shows an error toast instead of silently swallowing, andremove()reports a refused delete inline instead of leaving the sidebar open without feedback.fix(attachments): refetch when the open secret changes— the sidebar swaps the route's:idwithout remounting, so AttachmentPanel'smounted()-only fetch kept showing the first opened secret's attachments; asecretIdwatcher resets and refetches. New component regression test.l10n(walkthrough): translate the getting-started tour into all locales— the manifest tour was translated only in Dutch; all 36 other locales gained the tour strings and the menu labels it references (with wrong-language label corrections in cs/sk/sl/mk/be/uk/ca/sv/lb/rm).Review notes
Version000035) has been executed and verified on the shared dev instance.