refactor(ui): homogenize Account, Organization detail, and Admin pages#4187
Merged
Conversation
Options API dialog with simple yes/no, typed-confirmation gate, loading mode, custom confirmColor/titleClass, and default slot override.
…coreConfirmDialog
…ow/v-col + v-table hover
…ify labels, no inline styles
…ence with Account+Org Drop inline v-tabs, activeTab data, $route watcher, isValidTab, tabTo, resolveActiveTab, extraTabs computed. Replace with CoreSurfaceTabBar receiving allTabs (built-in + config extras) and adminCan predicate. Matches user.view.vue + organization.detail.component.vue pattern.
…mDialog with org-name gate
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughThe PR extracts two reusable Vue components ( ChangesCore and admin infrastructure refactoring
Organization and user component dialog and avatar migration
Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Account, Organization detail, and Admin surfaces to share consistent “page chrome” (header + tab bar via CoreSurfaceTabBar), and centralizes destructive confirmations behind a new shared coreConfirmDialog. It also introduces coreAvatarUploader to standardize avatar uploads without inline styles and updates multiple views/tests to enforce these conventions.
Changes:
- Added shared core components:
coreConfirmDialog(typed-gate destructive confirm) andcoreAvatarUploader(badge-based camera overlay). - Refactored Admin layout/sub-views to let
admin.layout.vueown chrome (container/header/tabs) and added breadcrumb publishing viaadmin.store. - Replaced multiple inline
<v-dialog>confirm blocks with<coreConfirmDialog>and updated/added unit tests to enforce the patterns (nov-dialog, nov-container, no inline styles in targeted files).
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/users/views/user.profile.view.vue | Replaces delete-account v-dialog with coreConfirmDialog typed gate. |
| src/modules/users/views/user.organizations.view.vue | Replaces leave-org v-dialog with coreConfirmDialog; adds computed message. |
| src/modules/users/tests/user.profile.view.unit.tests.js | Updates tests and adds source-grep assertions for coreConfirmDialog. |
| src/modules/users/tests/user.profile.component.unit.tests.js | New tests verifying coreAvatarUploader integration and label/inline-style hygiene. |
| src/modules/users/tests/user.organizations.view.unit.tests.js | Adds confirm-dialog source-grep regression coverage. |
| src/modules/users/components/user.profile.component.vue | Switches to Vuetify label props and uses coreAvatarUploader (removes custom overlay/upload code). |
| src/modules/organizations/tests/organizations.detail.layout.unit.tests.js | Adds stubs and confirm-dialog regression assertions for org delete flow. |
| src/modules/organizations/components/organization.detail.component.vue | Replaces delete-org v-dialog with coreConfirmDialog typed gate. |
| src/modules/core/tests/core.confirmDialog.unit.tests.js | New unit tests for coreConfirmDialog behavior (rendering, typed gating, emit). |
| src/modules/core/tests/core.avatarUploader.unit.tests.js | New unit tests for coreAvatarUploader (render + upload emit). |
| src/modules/core/components/core.confirmDialog.component.vue | New shared destructive confirmation dialog component. |
| src/modules/core/components/core.avatarUploader.component.vue | New shared avatar uploader component. |
| src/modules/admin/views/admin.users.view.vue | Removes view-owned chrome and replaces delete v-dialog with coreConfirmDialog. |
| src/modules/admin/views/admin.user.view.vue | Removes nested header/container; publishes breadcrumb; uses coreConfirmDialog for delete. |
| src/modules/admin/views/admin.readiness.view.vue | Removes view-owned container chrome; keeps content-only card/table. |
| src/modules/admin/views/admin.organizations.view.vue | Removes view-owned container chrome; keeps datatable-only content. |
| src/modules/admin/views/admin.layout.vue | Delegates tabs to CoreSurfaceTabBar, adds breadcrumb mode and unified content wrapper. |
| src/modules/admin/views/admin.activity.view.vue | Removes view-owned chrome and inline styles; adds v-table hover and layout refactor. |
| src/modules/admin/tests/admin.users.view.unit.tests.js | Adds template “no v-container / uses coreConfirmDialog” regression checks. |
| src/modules/admin/tests/admin.user.view.unit.tests.js | New tests for chrome removal, breadcrumb lifecycle, and delete flow navigation. |
| src/modules/admin/tests/admin.store.unit.tests.js | Adds coverage for currentBreadcrumb + set/clear actions. |
| src/modules/admin/tests/admin.readiness.view.unit.tests.js | Adds “no v-container” regression check. |
| src/modules/admin/tests/admin.organizations.view.unit.tests.js | Adds “no v-container” regression check. |
| src/modules/admin/tests/admin.layout.unit.tests.js | Updates tests for CoreSurfaceTabBar delegation, banners ordering, breadcrumb mode. |
| src/modules/admin/tests/admin.activity.view.unit.tests.js | Adds regressions for no container, no inline styles, and v-table hover. |
| src/modules/admin/stores/admin.store.js | Adds currentBreadcrumb state and setBreadcrumb/clearBreadcrumb actions. |
| .gitignore | Ignores .worktrees/ directory. |
Comment on lines
+137
to
+143
| * @desc Organization name used as the typed-confirmation target for the | ||
| * delete dialog. Falls back to '' before the org loads — the | ||
| * dialog's `coreConfirmDialog` keeps the confirm button disabled. | ||
| * @returns {string} | ||
| */ | ||
| deleteConfirmTarget() { | ||
| return this.viewedOrganization?.name || ''; |
Comment on lines
+138
to
+139
| * delete dialog. Falls back to '' before the org loads — the | ||
| * dialog's `coreConfirmDialog` keeps the confirm button disabled. |
Comment on lines
+77
to
+79
| if (!u || (!u.firstName && !u.lastName && !u.email)) return; | ||
| const title = [u.firstName, u.lastName].filter(Boolean).join(' ') || u.email; | ||
| useAdminStore().setBreadcrumb({ title, titleClass: 'text-capitalize' }); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4187 +/- ##
=======================================
Coverage 99.55% 99.56%
=======================================
Files 31 31
Lines 1136 1140 +4
Branches 328 329 +1
=======================================
+ Hits 1131 1135 +4
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- publishBreadcrumb() now calls clearBreadcrumb() when the user record is null/blank, preventing stale title display during same-component navigation between /admin/users/:id routes (Copilot finding). - Add test: "clears breadcrumb when user resets to blank record". - Fix misleading JSDoc on deleteConfirmTarget() in organization.detail — the confirm button is guarded by v-if on the trigger, not by the empty confirmText fallback (Copilot finding).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/modules/admin/views/admin.layout.vue (1)
138-141: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete
clearErrorJSDoc with required annotations.This method needs a full header matching the project JSDoc contract.
♻️ Suggested patch
/** - * `@desc` Clear the global admin error banner. + * `@desc` Clear the global admin error banner. + * `@returns` {void} */ clearError() { useAdminStore().error = null; },As per coding guidelines, "
**/*.{js,ts,vue}: Every new or modified function must have a JSDoc header..." and "Every function must have JSDoc header with@paramand@returnsannotations."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/admin/views/admin.layout.vue` around lines 138 - 141, Add a complete JSDoc header for the clearError method named "clearError" that follows the project contract: include a brief `@desc`, the method name (e.g. `@method` clearError), and a `@returns` annotation specifying {void}; if your linter/project expects param annotations for consistency, include an explicit `@param` {void} [none] line or a note like `@param` none. Ensure the header appears immediately above the clearError() declaration and uses the same JSDoc style as other component methods (e.g., /** ... */).src/modules/admin/tests/admin.layout.unit.tests.js (1)
29-62: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc for the
mountLayouthelper function.
mountLayoutis a standalone utility function and needs the required JSDoc header.♻️ Suggested patch
-const mountLayout = (configOverrides = {}, routePath = '/admin/users') => +/** + * Mount AdminLayout with shared test stubs/mocks. + * `@param` {Object} [configOverrides={}] - Config overrides merged into global config mock. + * `@param` {string} [routePath='/admin/users'] - Mocked current route path. + * `@returns` {import('`@vue/test-utils`').VueWrapper} + */ +const mountLayout = (configOverrides = {}, routePath = '/admin/users') => mount(AdminLayout, {As per coding guidelines, "
**/*.{js,ts,vue}: Every new or modified function must have a JSDoc header with one-line description,@paramfor each argument, and@returnsfor any non-void return value."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/admin/tests/admin.layout.unit.tests.js` around lines 29 - 62, Add a JSDoc header for the mountLayout helper: a one-line description explaining it mounts AdminLayout for tests, an `@param` for configOverrides (object, optional, defaults to {}) and an `@param` for routePath (string, optional, defaults to '/admin/users'), and an `@returns` describing the mounted wrapper (the Vue test wrapper returned by mount). Place this JSDoc immediately above the mountLayout function declaration so linters and docs pick it up.src/modules/users/components/user.profile.component.vue (1)
130-145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete JSDoc tags for modified methods.
syncFormandsyncOrganizationswere modified and documented, but their headers still omit@returnsannotations.Suggested patch
/** * `@desc` Pull the form fields from a fresh `user` prop and reset dirty. * `@param` {object} u - The user record. + * `@returns` {void} */ syncForm(u) { @@ /** * `@desc` Build the `orgItems` list from `user.memberships` (preferred) or * the `organizations` prop (legacy flat format). + * `@returns` {void} */ syncOrganizations() {As per coding guidelines:
**/*.{js,ts,vue}: Every function must have JSDoc header with@paramand@returnsannotations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/users/components/user.profile.component.vue` around lines 130 - 145, The JSDoc for the modified methods is missing `@returns` annotations; update the JSDoc for syncForm and syncOrganizations to include a `@returns` {void} (or the correct return type) line so every function has both `@param` and `@returns` per guidelines — locate the JSDoc blocks above the syncForm and syncOrganizations methods in user.profile.component.vue and add the `@returns` tag (e.g., `@returns` {void}) with a short description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/admin/stores/admin.store.js`:
- Around line 128-134: Add JSDoc headers to the breadcrumb store actions: add a
JSDoc block above setBreadcrumb describing the function, include a `@param`
{Object|null} payload description and a `@returns` {void} tag, and add a JSDoc
block above clearBreadcrumb with no `@param` and a `@returns` {void} tag; place
these comments immediately before the setBreadcrumb and clearBreadcrumb function
declarations so they meet the project's requirement for `@param` and `@returns`
annotations.
In `@src/modules/admin/tests/admin.user.view.unit.tests.js`:
- Around line 36-55: Add a JSDoc header above the mountView function: a one-line
description of what the helper does, an `@param` for routeId (string, default
'u1') and an `@param` for initialUser (object|null) explaining pre-seeding the
store, and an `@returns` describing that the function returns a shallow-mounted
AdminUserView wrapper (the Vue test wrapper returned by shallowMount). Reference
the helper by name (mountView) and note that it also sets up Pinia and mocks
store methods (resetUser/getUser/updateUser/deleteUser) and router/$route in the
returned wrapper.
- Around line 41-44: The current test mocks replace store.resetUser with a no-op
which bypasses the real clear-then-fetch behavior and can produce false-positive
breadcrumb/state tests; change the mock for resetUser to a stateful mock (use
vi.fn().mockImplementation) that clears the store's internal user state (e.g.,
set store.user = null or delete the cached field) and then returns the same
Promise behavior as the real function so subsequent
getUser/updateUser/deleteUser logic exercises the clear-then-fetch flow; update
the other mocked spots mentioned (lines 85-99) the same way so tests observe
real state transitions rather than a no-op reset.
In `@src/modules/admin/views/admin.activity.view.vue`:
- Line 76: The table row currently only handles mouse clicks; make it
keyboard-accessible by adding tabindex="0" and role="button" to the <tr> with
`@keydown` handlers that invoke toggleActivityExpand(item._id || item.id) on Enter
and Space (prevent default on Space), and bind an aria-expanded attribute (e.g.,
:aria-expanded="isActivityExpanded(item._id || item.id)") so assistive tech
knows the state; ensure the existing `@click` still calls
toggleActivityExpand(item._id || item.id).
In `@src/modules/admin/views/admin.user.view.vue`:
- Around line 64-69: Add full JSDoc headers above the watcher handler and the
lifecycle hook: add a one-line description and an `@param` for the handler
function parameter `u` (e.g., `@param {UserType} u - updated user payload`), and
add a one-line description for `beforeUnmount` (no params). Since neither
function returns a value, omit `@returns` (only include `@returns` when non-void or
async). Place these JSDoc comments immediately above the `handler(u) {
this.publishBreadcrumb(u); }` watcher and above `beforeUnmount() {
useAdminStore().clearBreadcrumb(); }`.
- Around line 77-80: The current guard returns early when user 'u' is missing,
leaving the previous breadcrumb intact; update the branch handling falsy or
empty 'u' to explicitly clear the admin breadcrumb by calling
useAdminStore().setBreadcrumb with an empty title (or null) and appropriate
titleClass instead of returning, and keep the existing logic that sets the
composed title ([u.firstName, u.lastName]...) when a valid user exists; modify
the code around the existing check that references useAdminStore().setBreadcrumb
to perform the clear action before exiting.
In `@src/modules/admin/views/admin.users.view.vue`:
- Around line 14-19: Remove the inline :style binding on the membership v-chip
and replace it with a class-based approach: conditionally add a class (e.g.,
"clickable") via :class="{ clickable: membershipOrgId(m) }" on the same v-chip
and implement the cursor: pointer rule in the component's scoped CSS; ensure the
click handler navigateToMembershipOrg(m) and the helper membershipOrgId(m)
remain unchanged so behavior is preserved.
In `@src/modules/core/components/core.avatarUploader.component.vue`:
- Around line 52-71: Add JSDoc headers for the two uploader methods: document
triggerUpload with a one-line description and a `@returns` stating it returns
void, and document async onFile with a one-line description, a `@param` for the
event (e.g., HTMLInputElement or Event with target.files) and a `@returns`
indicating Promise<void>; ensure the JSDoc appears immediately above the
triggerUpload and onFile method declarations and describes the input event and
the emitted outcomes (uploaded/error) as part of the comments.
In `@src/modules/core/components/core.confirmDialog.component.vue`:
- Around line 69-71: The component is missing required JSDoc headers for several
functions (data, canConfirm, the watcher for modelValue, onCancel, and
onConfirm); add a one-line description for each function and include `@param` tags
for every parameter and `@returns` for any non-void return value. Update the
data() function to have a JSDoc describing the returned state and `@returns` type,
add a JSDoc above the canConfirm computed/method describing its purpose and
`@returns` boolean, add a JSDoc for the modelValue watcher explaining the watched
param with `@param` (newVal: type, oldVal: type) and any return behavior, and add
JSDoc blocks for onCancel and onConfirm with a one-line description and `@param`
entries for any event or payload and `@returns` if they return a value; place
these JSDoc comments directly above the corresponding functions (data,
canConfirm, watch: { modelValue(...) }, onCancel, onConfirm).
In `@src/modules/organizations/tests/organizations.detail.layout.unit.tests.js`:
- Around line 542-548: The assertion is tautological; instead simulate the
null-organization fallback and assert the computed returns an empty string: use
mountLayout to create the wrapper, set wrapper.vm.viewedOrganization = null (or
adjust the mock used by mountLayout to return null), then assert
expect(wrapper.vm.deleteConfirmTarget).toBe(''); reference the computed
deleteConfirmTarget, the viewedOrganization property, and the mountLayout helper
to locate and change the test.
In `@src/modules/users/tests/user.profile.component.unit.tests.js`:
- Around line 13-14: Add a JSDoc header above the standalone helper function
mountProfile describing its purpose in one line, include an `@param` entry for the
props parameter (object, optional) and an `@returns` entry describing the returned
Vue test wrapper (the result of mount(userProfile, {...})). Update the JSDoc to
match the function signature and return type so it satisfies the project rule
requiring one-line description, `@param`, and `@returns` for new/modified functions.
---
Outside diff comments:
In `@src/modules/admin/tests/admin.layout.unit.tests.js`:
- Around line 29-62: Add a JSDoc header for the mountLayout helper: a one-line
description explaining it mounts AdminLayout for tests, an `@param` for
configOverrides (object, optional, defaults to {}) and an `@param` for routePath
(string, optional, defaults to '/admin/users'), and an `@returns` describing the
mounted wrapper (the Vue test wrapper returned by mount). Place this JSDoc
immediately above the mountLayout function declaration so linters and docs pick
it up.
In `@src/modules/admin/views/admin.layout.vue`:
- Around line 138-141: Add a complete JSDoc header for the clearError method
named "clearError" that follows the project contract: include a brief `@desc`, the
method name (e.g. `@method` clearError), and a `@returns` annotation specifying
{void}; if your linter/project expects param annotations for consistency,
include an explicit `@param` {void} [none] line or a note like `@param` none. Ensure
the header appears immediately above the clearError() declaration and uses the
same JSDoc style as other component methods (e.g., /** ... */).
In `@src/modules/users/components/user.profile.component.vue`:
- Around line 130-145: The JSDoc for the modified methods is missing `@returns`
annotations; update the JSDoc for syncForm and syncOrganizations to include a
`@returns` {void} (or the correct return type) line so every function has both
`@param` and `@returns` per guidelines — locate the JSDoc blocks above the syncForm
and syncOrganizations methods in user.profile.component.vue and add the `@returns`
tag (e.g., `@returns` {void}) with a short description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b591d23d-b7b1-4795-aa2f-73c01708cbe9
📒 Files selected for processing (27)
.gitignoresrc/modules/admin/stores/admin.store.jssrc/modules/admin/tests/admin.activity.view.unit.tests.jssrc/modules/admin/tests/admin.layout.unit.tests.jssrc/modules/admin/tests/admin.organizations.view.unit.tests.jssrc/modules/admin/tests/admin.readiness.view.unit.tests.jssrc/modules/admin/tests/admin.store.unit.tests.jssrc/modules/admin/tests/admin.user.view.unit.tests.jssrc/modules/admin/tests/admin.users.view.unit.tests.jssrc/modules/admin/views/admin.activity.view.vuesrc/modules/admin/views/admin.layout.vuesrc/modules/admin/views/admin.organizations.view.vuesrc/modules/admin/views/admin.readiness.view.vuesrc/modules/admin/views/admin.user.view.vuesrc/modules/admin/views/admin.users.view.vuesrc/modules/core/components/core.avatarUploader.component.vuesrc/modules/core/components/core.confirmDialog.component.vuesrc/modules/core/tests/core.avatarUploader.unit.tests.jssrc/modules/core/tests/core.confirmDialog.unit.tests.jssrc/modules/organizations/components/organization.detail.component.vuesrc/modules/organizations/tests/organizations.detail.layout.unit.tests.jssrc/modules/users/components/user.profile.component.vuesrc/modules/users/tests/user.organizations.view.unit.tests.jssrc/modules/users/tests/user.profile.component.unit.tests.jssrc/modules/users/tests/user.profile.view.unit.tests.jssrc/modules/users/views/user.organizations.view.vuesrc/modules/users/views/user.profile.view.vue
Comment on lines
+41
to
+44
| store.resetUser = vi.fn(); | ||
| store.getUser = vi.fn().mockResolvedValue(); | ||
| store.updateUser = vi.fn().mockResolvedValue(); | ||
| store.deleteUser = vi.fn().mockResolvedValue(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make resetUser mock stateful to avoid false-positive breadcrumb tests.
At Line 41, mocking resetUser as a no-op bypasses the real clear-then-fetch flow, which can mask stale breadcrumb/state regressions.
Suggested test-hardening change
- store.resetUser = vi.fn();
+ store.resetUser = vi.fn(() => { store.user = null; });Also applies to: 85-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/admin/tests/admin.user.view.unit.tests.js` around lines 41 - 44,
The current test mocks replace store.resetUser with a no-op which bypasses the
real clear-then-fetch behavior and can produce false-positive breadcrumb/state
tests; change the mock for resetUser to a stateful mock (use
vi.fn().mockImplementation) that clears the store's internal user state (e.g.,
set store.user = null or delete the cached field) and then returns the same
Promise behavior as the real function so subsequent
getUser/updateUser/deleteUser logic exercises the clear-then-fetch flow; update
the other mocked spots mentioned (lines 85-99) the same way so tests observe
real state transitions rather than a no-op reset.
Comment on lines
+542
to
+548
| it('deleteConfirmTarget computed returns empty string when org is not yet loaded', () => { | ||
| // Temporarily override the mock to return null | ||
| const wrapper = mountLayout(); | ||
| // Patch the computed indirectly by checking the fallback logic via vm | ||
| // viewedOrganization is 'Acme' in this mock; test the fallback via direct logic check | ||
| expect(wrapper.vm.deleteConfirmTarget).toBe(wrapper.vm.viewedOrganization?.name || ''); | ||
| }); |
There was a problem hiding this comment.
Make the fallback-path assertion meaningful.
Line 547 is tautological (x === x || '') and won’t catch regressions in the null-organization path.
Suggested change
it('deleteConfirmTarget computed returns empty string when org is not yet loaded', () => {
- // Temporarily override the mock to return null
- const wrapper = mountLayout();
- // Patch the computed indirectly by checking the fallback logic via vm
- // viewedOrganization is 'Acme' in this mock; test the fallback via direct logic check
- expect(wrapper.vm.deleteConfirmTarget).toBe(wrapper.vm.viewedOrganization?.name || '');
+ const result = OrganizationDetailComponent.computed.deleteConfirmTarget.call({
+ viewedOrganization: null,
+ });
+ expect(result).toBe('');
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/organizations/tests/organizations.detail.layout.unit.tests.js`
around lines 542 - 548, The assertion is tautological; instead simulate the
null-organization fallback and assert the computed returns an empty string: use
mountLayout to create the wrapper, set wrapper.vm.viewedOrganization = null (or
adjust the mock used by mountLayout to return null), then assert
expect(wrapper.vm.deleteConfirmTarget).toBe(''); reference the computed
deleteConfirmTarget, the viewedOrganization property, and the mountLayout helper
to locate and change the test.
- admin.users.view: replace inline :style binding on membership chips with :class="cursor-pointer" (no-inline-style convention, CR finding) - admin.activity.view: add keyboard semantics to expandable rows (tabindex=0, role=button, aria-expanded, @keydown.enter+.space) + test (CR finding) - organizations.detail layout test: fix tautological assertion for deleteConfirmTarget null fallback (CR finding) - admin.store: add full JSDoc (@param/@returns) to setBreadcrumb + clearBreadcrumb - admin.layout: add @returns {void} to clearError - admin.user.view: add JSDoc to user watcher handler + beforeUnmount - admin.user.view test: add JSDoc to mountView helper - admin.layout test: add JSDoc to mountLayout helper - core.avatarUploader: add JSDoc to triggerUpload + onFile - core.confirmDialog: add JSDoc to data, canConfirm, modelValue watcher, onCancel, onConfirm; document the intentional non-self-close design - user.profile.component: add @returns {void} to syncForm + syncOrganizations - user.profile.component test: add JSDoc to mountProfile helper
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
Addressed all findings in follow-up commits (JSDoc, inline style→class, keyboard a11y, stale breadcrumb fix, tautological test fix)
PierreBrisorgueil
added a commit
that referenced
this pull request
May 21, 2026
Splits the chrome primitive into two specialised components so each page gets the spacing it actually needs: - `PageHeader` (existing, simplified): content-sized header for list views and simple pages (Tasks, Scraps, etc.). Drops the 56px min/max-height enforcement and removes the dead `#tabs` slot path (unused since the CoreSurfaceTabBar sibling refactor in #4187/#4188). - `CorePageHeaderTabs` (new): bundles a PageHeader + CoreSurfaceTabBar and enforces the 56px height externally via a scoped `:deep()` rule. Used on tabbed surfaces (Account, Organization detail, Admin) where the title↔ breadcrumb rhythm matters across route transitions. The new primitive accepts `tabs`, `can`, `basePath`, and `hideTabs` for the tab bar, plus passthrough of icon/title/subtitle/avatar/actions/breadcrumb slots and props to the inner PageHeader. Migrates 3 callsites: - user.view.vue (Account) - admin.layout.vue (with hideTabs in breadcrumb mode) - organization.detail.component.vue Visual impact: pages without tabs (e.g. /tasks) no longer pay the 56px chrome tax; tabbed surfaces preserve the route-transition rhythm.
PierreBrisorgueil
added a commit
that referenced
this pull request
May 21, 2026
…refactor Adds a new migration entry above the previous chrome convergence entry covering the changes in this batch: - PageHeader is content-sized (56px enforcement removed) - #tabs slot on PageHeader dropped (dead since #4187) - mx-4 on v-tabs inside CoreSurfaceTabBar (horizontal alignment with title) - New CorePageHeaderTabs primitive that bundles PageHeader + SurfaceTabBar and re-enforces the 56px rhythm for tabbed surfaces - pb-0 on the tabbed-layout container to collapse the gap below tabs - Empty-state v-row regression pattern (guard the populated row with v-if) Downstream impact: minimal — only projects with custom tabbed layouts need to migrate to CorePageHeaderTabs; default consumers get the new convention via /update-stack automatically.
3 tasks
PierreBrisorgueil
added a commit
that referenced
this pull request
May 21, 2026
…erTabs) (#4190) * refactor(ui): tabs align with default sidenav offset + tighten gap below tabs - CoreSurfaceTabBar: add `mx-4` to `<v-tabs>` so the tab strip aligns horizontally with PageHeader's title icon (both now use the same 16px inset inside `v-container fluid`). - user.view.vue + admin.layout.vue: add `pb-0` on the parent `<v-container fluid>` to collapse ~16px of dead space between the tab underline and the routed child's top edge. Child views own their own top padding via their own `<v-container fluid>` + `pa-2` row. Visual impact (Account, Org detail, Admin surfaces): - Tabs now align with the section title icon instead of being 16px to the left - Vertical gap below tabs drops from ~40px to ~24px * refactor(ui): apply pb-0 to organization detail container Extends the tabs-spacing-alignment fix to organization.detail.component.vue which had the same `<v-container fluid>` parent pattern as user.view.vue and admin.layout.vue. Brings /users/organizations/:id/* into visual parity with /users/profile and /admin/* surfaces. * refactor(core): split PageHeader + CorePageHeaderTabs Splits the chrome primitive into two specialised components so each page gets the spacing it actually needs: - `PageHeader` (existing, simplified): content-sized header for list views and simple pages (Tasks, Scraps, etc.). Drops the 56px min/max-height enforcement and removes the dead `#tabs` slot path (unused since the CoreSurfaceTabBar sibling refactor in #4187/#4188). - `CorePageHeaderTabs` (new): bundles a PageHeader + CoreSurfaceTabBar and enforces the 56px height externally via a scoped `:deep()` rule. Used on tabbed surfaces (Account, Organization detail, Admin) where the title↔ breadcrumb rhythm matters across route transitions. The new primitive accepts `tabs`, `can`, `basePath`, and `hideTabs` for the tab bar, plus passthrough of icon/title/subtitle/avatar/actions/breadcrumb slots and props to the inner PageHeader. Migrates 3 callsites: - user.view.vue (Account) - admin.layout.vue (with hideTabs in breadcrumb mode) - organization.detail.component.vue Visual impact: pages without tabs (e.g. /tasks) no longer pay the 56px chrome tax; tabbed surfaces preserve the route-transition rhythm. * fix(tasks): align empty state row spacing with populated state The empty-state v-row was missing `pa-2 mt-0` (so Vuetify's default v-row margin applied) and the card carried `ma-6` (24px all-sides margin), which together added ~36px of dead space above the "No tasks yet" card compared to the populated state. Drop `ma-6` and apply the same `pa-2 mt-0` row classes as the populated branch — empty state now sits flush with where task cards would render. * fix(tasks): harmonize empty state card with /task layout Drop `align="start" justify="center"` on the v-row and align card padding (pa-8 → pa-6) so the empty-state card sits at the exact same top/left as the form card on /task. Keeps `text-center` for icon + label centering inside the card. * fix(tasks): hide populated v-row when task list is empty The populated state's v-row had no v-if guard, so when the task list was empty it still rendered (just without children) — taking ~16-24px of vertical space (pa-2 + default v-row margins) on top of the empty-state row below. Empty-state card now sits at the same top position as the populated cards (and /task form card). * docs(migrations): document PageHeader split + tabs spacing/alignment refactor Adds a new migration entry above the previous chrome convergence entry covering the changes in this batch: - PageHeader is content-sized (56px enforcement removed) - #tabs slot on PageHeader dropped (dead since #4187) - mx-4 on v-tabs inside CoreSurfaceTabBar (horizontal alignment with title) - New CorePageHeaderTabs primitive that bundles PageHeader + SurfaceTabBar and re-enforces the 56px rhythm for tabbed surfaces - pb-0 on the tabbed-layout container to collapse the gap below tabs - Empty-state v-row regression pattern (guard the populated row with v-if) Downstream impact: minimal — only projects with custom tabbed layouts need to migrate to CorePageHeaderTabs; default consumers get the new convention via /update-stack automatically.
5 tasks
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.
Summary
Homogenizes the page chrome and destructive-confirmation pattern across the three account-adjacent surfaces in devkit Vue: Account (
/users/*), Organization detail (/users/organizations/:id/*and/admin/organizations/:id/*), and Admin (/admin/*).Builds on top of the upstream chrome refactor (#4183 —
corePageTabs, #4184 — org tabs, #4185 — Account chrome viaCoreSurfaceTabBar) by:CoreSurfaceTabBarconvention to admin (Task 4b).coreConfirmDialog(typed-gate destructive confirmation) andcoreAvatarUploader(v-badge-based camera overlay, no inline styles).<v-dialog>blocks withcoreConfirmDialog.v-container/PageHeaderso the layout owns chrome.style="…"attributes fromadmin.activity.view.vue(and ensuring zero introduced in the other touched files).Conventions locked in
corePageHeader's#tabsslot, usingCoreSurfaceTabBar. No surroundingv-card.admin.layout.vueownsv-container,PageHeader, banners, content padding. Sub-views render content only. Detail pages publish their title viauseAdminStore().setBreadcrumb({ title, titleClass })and clear on unmount.<coreConfirmDialog>. Simple yes/no by default; optionalconfirm-texttyped-gate (e.g. type DELETE / type {orgName}).<style>blocks, zero newstyle="…"attributes anywhere in touched files. Inline overlays replaced byv-badge; width constraints via Vuetifymax-widthprops; row hover via<v-table hover>.What changed
New shared core components
core.confirmDialog.component.vue(+ test, 4 cases)core.avatarUploader.component.vue(+ test, 2 cases)Admin layout
admin.layout.vue→ banners on top, tabs delegated toCoreSurfaceTabBar, breadcrumb support for detail mode, single.admin-content pa-4content wrapper.admin.store.js→currentBreadcrumbstate +setBreadcrumb/clearBreadcrumbactions.Admin sub-views
admin.users.view.vue→ drop own container, swap delete v-dialog → coreConfirmDialog.admin.organizations.view.vue→ drop own container.admin.readiness.view.vue→ drop own container.admin.activity.view.vue→ drop own container, drop ALL inline styles (max-width on filters, cursor:pointer on rows), usev-row dense+v-col cols+<v-table hover>.admin.user.view.vue→ drop nested<PageHeader>, publish breadcrumb on mount, clear on unmount, swap delete v-dialog → coreConfirmDialog.Users module
user.profile.component.vue→ usecoreAvatarUploader(drops 30+ lines of hand-rolled overlay), native Vuetifylabel="…"props on all 5 inputs (drops manual<label class="text-label-large">headers), drop unused imports + deadisActiveOrgmethod.user.profile.view.vue→ swap Delete Account v-dialog → coreConfirmDialog withconfirm-text="DELETE"typed-gate. DropdeleteConfirmInputdata field.user.organizations.view.vue→ swap Leave Org v-dialog → coreConfirmDialog with org-name interpolation via computed.Organizations module
organization.detail.component.vue→ swap Delete Organization v-dialog → coreConfirmDialog withconfirm-text="{orgName}"typed-gate. DropdeleteConfirmNamedata field.Tests
All test files updated in lockstep (source-grep regression assertions for "no v-dialog" / "uses coreConfirmDialog" / "no v-container" / "no inline style" / "uses CoreSurfaceTabBar" / "v-table has hover"). Two new test files for the shared core components. New tests for
admin.user.view(template chrome + breadcrumb lifecycle + delete flow),user.profile.component,organization.detail.component.--omit=dev).Visual smoke (Chrome DevTools MCP)
GREEN_WITH_SKIPS — 6 of 7 surfaces visually confirmed clean (admin/users, admin/users/:id with breadcrumb + delete dialog, admin/readiness, admin/activity hover, users/profile with coreAvatarUploader + Delete Account typed-gate, users/organizations/:id/general with org-name typed-gate). 1 surface (users/organizations Leave dialog) source-verified + unit-test-covered; UI trigger requires a non-owner org membership not present in the test DB.
Downstream propagation
After merge, this propagates to all downstream Vue projects via
/update-stack(pierreb_vue, comes_vue, trawl_vue, montaine_vue, ism_vue). Per the project'sfeedback_shim_defer_downstreamrule, propagation is on-demand via/update-project, not automatic.Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores