VPR-59 [3/4] CMS migration: management SPA (frontend)#253
Open
rlorenzo wants to merge 15 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Completes the CMS migration by landing the Vue 3 + Quasar “management SPA” for CMS tools (files, content blocks, left-nav menus), alongside shared SPA infrastructure (router scroll/focus behavior), reusable list/table composables, shared UI components, and an expanded frontend test suite.
Changes:
- Added CMS management pages/components (hub, files audit, bulk encrypt, content history/diff, left-nav menu management) plus shared CMS composables/types and utilities.
- Introduced shared frontend building blocks:
createSpaRouter(hash + query-aware scroll),useRouteFocusrefinements,useReorder+SortableList, and server-paged table composables with URL-synced filters. - Updated global styles/tokens/utilities for surfaces, focus rings, and responsive list/table/card layouts; minor Effort audit filter layout adjustments.
Reviewed changes
Copilot reviewed 85 out of 85 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| VueApp/src/styles/colors.css | Adds surface/diff/focus tokens and bg utilities for secondary palette colors. |
| VueApp/src/styles/base.css | Adds global a11y/layout helpers (focus, list headers, dialog sizing, card-mode table styling). |
| VueApp/src/Students/router/index.ts | Switches router factory import to create-spa-router. |
| VueApp/src/shared/createSpaRouter.ts | Removes old router factory module (renamed/replaced). |
| VueApp/src/shared/create-spa-router.ts | New router factory with improved base-url normalization and scroll behavior (hash + query-aware). |
| VueApp/src/shared/tests/create-spa-router.test.ts | Tests router scroll behavior and route-focus behavior. |
| VueApp/src/layouts/ViperLayout.vue | Adds “Skip to section menu” skip link when left nav exists. |
| VueApp/src/layouts/LeftNav.vue | Improves left-nav a11y (skip target focusability, roles, landmark labeling). |
| VueApp/src/Effort/router/index.ts | Switches router factory import to create-spa-router. |
| VueApp/src/Effort/pages/AuditList.vue | Reworks search + clear-filters placement to match CMS audit UX. |
| VueApp/src/Effort/components/EffortLeftNav.vue | Makes left nav skip target focusable (tabindex=-1). |
| VueApp/src/CTS/router/index.ts | Switches router factory import to create-spa-router. |
| VueApp/src/config/colors.ts | Extends accessible text-color logic for the new arboretum background color. |
| VueApp/src/Computing/router/index.ts | Switches router factory import to create-spa-router. |
| VueApp/src/composables/ViperFetch.ts | Adds postForm/putForm helpers for multipart uploads; minor object shorthand cleanup. |
| VueApp/src/composables/use-route-focus.ts | Avoids focus-steal on same-path navigations (query-only/hash-only). |
| VueApp/src/composables/use-reorder.ts | New reorder composable (move math + “just moved” highlight state). |
| VueApp/src/composables/tests/use-reorder.test.ts | Unit tests for useReorder. |
| VueApp/src/components/SortableList.vue | New shared sortable list component (drag + buttons + a11y announcements + animations). |
| VueApp/src/components/BreadcrumbHeading.vue | New breadcrumb-style <h1> component for subpages. |
| VueApp/src/components/tests/sortable-list.test.ts | Unit tests for SortableList. |
| VueApp/src/CMS/types/index.ts | Expands CMS TypeScript types for files, blocks, nav, audit/history, and activity rail. |
| VueApp/src/CMS/router/routes.ts | Defines CMS SPA routes and shared home-permission set. |
| VueApp/src/CMS/router/index.ts | Uses new router factory and adds CMS area-root canonicalization to Home. |
| VueApp/src/CMS/pages/LeftNavMenus.vue | New CMS page: list/manage left-nav menus (filters + add dialog + delete). |
| VueApp/src/CMS/pages/FileAuditLog.vue | New CMS page: server-paged file audit trail with URL-synced filters. |
| VueApp/src/CMS/pages/ContentBlockHistory.vue | New CMS page: cross-block edit history with diff viewer. |
| VueApp/src/CMS/pages/CmsHome.vue | Redesigns CMS hub with permission-scoped cards and purge-soon warning. |
| VueApp/src/CMS/pages/BulkEncrypt.vue | New CMS tool page: server-paged list for bulk encryption. |
| VueApp/src/CMS/file-types.ts | Centralizes accepted file extension allow-list for uploads. |
| VueApp/src/CMS/composables/use-url-filtered-table.ts | New composable: URL-synced filters layered over server paging. |
| VueApp/src/CMS/composables/use-server-table.ts | New composable: server-paged QTable core with stale-response guard. |
| VueApp/src/CMS/composables/use-content-diff-viewer.ts | New shared diff-viewer state/helpers for ContentDiffDialog consumers. |
| VueApp/src/CMS/components/StatusIcon.vue | New component: tooltip + sr-only label pattern for status icons. |
| VueApp/src/CMS/components/RecentActivity.vue | New component: CMS hub activity rail + latest-diff helpers. |
| VueApp/src/CMS/components/PersonSelector.vue | New component: server-searched people selector with stale-response guard. |
| VueApp/src/CMS/components/PermissionSelector.vue | New component: lazy-loaded permission selector with client-side filtering. |
| VueApp/src/CMS/components/PermissionChips.vue | New component: compact permission/person-count chip rendering. |
| VueApp/src/CMS/components/ModifiedStamp.vue | New component: shared “MM/DD/YY by user” stamp for table + card modes. |
| VueApp/src/CMS/components/ListCardField.vue | New component: label/value rows for QTable card/grid mode. |
| VueApp/src/CMS/components/ListCard.vue | New component: shared QTable card/grid wrapper for CMS list pages. |
| VueApp/src/CMS/components/LinkCollections.vue | Optimizes grouping/filtering behavior and adjusts filter UX. |
| VueApp/src/CMS/components/Link.vue | Uses StatusBadge for tags and updates tag palette to non-semantic brand roles. |
| VueApp/src/CMS/components/LeftNavMenuSettingsFields.vue | New shared settings form for left-nav menu create/edit. |
| VueApp/src/CMS/components/LeftNavMenuDialog.vue | New create-menu dialog with validation + unsaved-changes guard. |
| VueApp/src/CMS/components/EditButton.vue | New shared edit action button component for tables/cards. |
| VueApp/src/CMS/components/DeleteRestoreButtons.vue | New shared delete/restore action button component. |
| VueApp/src/CMS/components/DateRangeFilter.vue | New shared from/to date filter inputs. |
| VueApp/src/CMS/components/ContentDiffDialog.vue | New diff dialog for content history comparisons. |
| VueApp/src/CMS/components/ContentBlock.vue | Fixes content fetch behavior (avoid duplicate/deep watch) and handles fetch failure. |
| VueApp/src/CMS/components/ActivityRow.vue | New component: recent-activity row layout with safe whole-row click behavior. |
| VueApp/src/CMS/tests/use-url-filtered-table.test.ts | Tests URL-synced server table behavior and equality-guard. |
| VueApp/src/CMS/tests/use-server-table.test.ts | Tests server table behavior and stale-response guard. |
| VueApp/src/CMS/tests/url.test.ts | Tests CMS URL safety helpers (scheme allow/block, malformed handling). |
| VueApp/src/CMS/tests/test-utils.ts | CMS test harness utilities (Quasar/Pinia/router + seeded permissions). |
| VueApp/src/CMS/tests/router-canonicalization.test.ts | Tests CMS /CMS/ → Home canonicalization across permission shapes. |
| VueApp/src/CMS/tests/person-selector.test.ts | Tests PersonSelector min-chars gate and stale-response guard. |
| VueApp/src/CMS/tests/permission-selector.test.ts | Tests PermissionSelector lazy load + filtering + null-clear behavior. |
| VueApp/src/CMS/tests/modified-stamp.test.ts | Tests ModifiedStamp formatting and dual render modes. |
| VueApp/src/CMS/tests/link.test.ts | Tests Link URL safety rendering and tag color cycling behavior. |
| VueApp/src/CMS/tests/left-nav-menus.test.ts | Tests left-nav menu list behavior (filters, deep-link add dialog, delete flow). |
| VueApp/src/CMS/tests/left-nav-menu-dialog.test.ts | Tests create-menu dialog validation and POST success/failure behavior. |
| VueApp/src/CMS/tests/file-form-dialog-conflict.test.ts | Tests file upload name-conflict flows and multipart upload helpers usage. |
| VueApp/src/CMS/tests/date-range-filter.test.ts | Tests DateRangeFilter v-model and change emission. |
| VueApp/src/CMS/tests/content-diff-dialog.test.ts | Tests ContentDiffDialog close/legend/empty behaviors. |
| VueApp/src/CMS/tests/content-block-history.test.ts | Tests history list URL sync + diff viewer behaviors. |
| VueApp/src/CMS/tests/content-block-edit-diff.test.ts | Tests ContentBlockEdit diff-against-current behaviors. |
| VueApp/src/CMS/tests/cms-home.test.ts | Tests CMS hub permission-gated sections and purge warning behavior. |
| VueApp/src/ClinicalScheduler/router/index.ts | Switches router factory import to create-spa-router. |
| VueApp/src/CAHFS/router/index.ts | Switches router factory import to create-spa-router. |
This was referenced Jul 3, 2026
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds CMS management pages and workflows, reusable upload, diff, routing, table, sorting, accessibility, and rich-text infrastructure, shared audit coloring, and extensive component, page, composable, and router tests. ChangesCMS platform
Shared infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
VueApp/src/Effort/router/index.ts (1)
13-52: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEval-permission load failure permanently locks the user out for the session.
loadEvalPermissions()has nocatcharound theget()call, and the entire "check/load missing Eval permissions" block (lines 44-51) only runs when!isInternalNavigation(line 36). OnceuserStore.isLoggedInistrue, every later internal navigation skips this block entirely — so if the first fetch fails or returnssuccess: false,hasEvalPermissionsnever becomestrueagain for the rest of the session, and the user is silently redirected away from everySVMSecure.Eval-gated route with no retry path short of a full page reload.Decouple the eval-permission check from the internal-navigation re-auth gate, and handle fetch failures explicitly so a transient error doesn't permanently disable future attempts.
🩹 Proposed direction
router.beforeEach(async (to, from) => { const userStore = useUserStore() - // Skip re-authentication for in-app navigations (tab switches, course-to-course). - // The user is already logged in and permissions are loaded; re-calling requireLogin - // Would overwrite the permission array and cause a visible flash. const isInternalNavigation = from.matched.length > 0 && userStore.isLoggedIn if (!isInternalNavigation) { const { requireLogin } = useRequireLogin(to) const loginResult = await requireLogin(true, "SVMSecure.Effort") if (loginResult !== null && !loginResult) { return false } + } - // Eval permissions are in a separate area, so they aren't loaded by requireLogin - const existingPermissions = userStore.userInfo?.permissions ?? [] - const hasEvalPermissions = existingPermissions.some((p: string) => p.startsWith("SVMSecure.Eval")) - if (!hasEvalPermissions) { - if (!evalPermissionsPromise) { - evalPermissionsPromise = loadEvalPermissions() - } - await evalPermissionsPromise - } + // Re-check on every navigation (not just external ones) so a transient failure can be retried. + const existingPermissions = userStore.userInfo?.permissions ?? [] + const hasEvalPermissions = existingPermissions.some((p: string) => p.startsWith("SVMSecure.Eval")) + if (!hasEvalPermissions) { + if (!evalPermissionsPromise) { + evalPermissionsPromise = loadEvalPermissions() + } + await evalPermissionsPromise }And add error handling in
loadEvalPermissions(e.g.catch+ log) rather than lettingget()rejections propagate unhandled into the guard.🤖 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 `@VueApp/src/Effort/router/index.ts` around lines 13 - 52, The eval-permission refresh in router.beforeEach is currently tied to the non-internal navigation path, so a failed load can permanently prevent future retries for the session. Move the SVMSecure.Eval permission check/load logic out of the isInternalNavigation gate so it still runs when needed on later navigations, and update loadEvalPermissions() to catch errors from useFetch().get() and log them instead of letting rejections escape. Keep the retry latch reset in loadEvalPermissions() so future attempts can refetch after a failure.VueApp/src/CMS/components/LinkCollections.vue (1)
16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
lt-smheader visibility choice.
header-class="bg-grey-2 lt-sm"uses Quasar's visibility utility class, which hides the header entirely onsm+ screens. This is only correct becausefiltersExpandedComputed(line 138-139) forces the panel open for$q.screen.gt.xs, so the toggle header is unnecessary abovexs. That relationship isn't obvious from this line alone and reads like a bug (hiding the "Filters" label/toggle on desktop) — a short comment here would prevent a future "fix" that reintroduces the header unnecessarily or removes the force-open logic without updating this class.💬 Suggested comment
+ <!-- Panel is force-expanded above xs (see filtersExpandedComputed), so the + toggle header is only shown/needed on xs screens. --> <q-expansion-item v-model="filtersExpandedComputed" icon="filter_list" label="Filters" header-class="bg-grey-2 lt-sm"🤖 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 `@VueApp/src/CMS/components/LinkCollections.vue` around lines 16 - 22, Add a short inline comment near the q-expansion-item header-class in LinkCollections.vue explaining that lt-sm intentionally hides the header on sm+ screens because filtersExpandedComputed forces the panel open for $q.screen.gt.xs. Reference filtersExpandedComputed and the q-expansion-item so future edits don’t remove this coupling or “fix” the hidden desktop header.VueApp/src/CMS/pages/ManageLinkCollections.vue (1)
512-563: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-baseline after a partial tag-save failure recovery.
handleTagFailurereloads tags and resetsdraftTags/deletedTagIdsto the true server state, but never callssetCollectionBaseline(). If any tag mutation succeeded before the failing step (e.g. a delete went through before an add failed), the resynceddraftTagsnow differs from the baseline captured when the dialog opened. Closing afterward will spuriously trigger "Unsaved Changes" even though the displayed state is already the current server truth.🛠️ Proposed fix
async function handleTagFailure(message: string) { $q.notify({ type: "negative", message }) await loadTags() draftTags.value = collectionTags.value.map((t) => ({ ...t })) deletedTagIds.value = [] + setCollectionBaseline() }🤖 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 `@VueApp/src/CMS/pages/ManageLinkCollections.vue` around lines 512 - 563, The partial tag-save recovery in handleTagFailure reloads server tags and resets draftTags/deletedTagIds, but it leaves the original baseline stale. Update handleTagFailure in ManageLinkCollections.vue to also re-baseline the dialog state after loadTags() by calling setCollectionBaseline() so the refreshed draft state matches the true server state. Keep this in the same recovery path used by the tag mutation loop (delete/add/order failures) so closing the dialog does not trigger a false Unsaved Changes warning.
♻️ Duplicate comments (3)
VueApp/src/CMS/components/LeftNavMenuDialog.vue (1)
53-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSame create-action color inconsistency as
LeftNavEdit.vue."Create Menu" here also uses
color="primary"rather thanpositive; see the corresponding comment onLeftNavEdit.vue(lines 35-50) for the guideline mapping and the sibling buttons that follow it.🤖 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 `@VueApp/src/CMS/components/LeftNavMenuDialog.vue` around lines 53 - 60, The Create Menu action in LeftNavMenuDialog.vue uses the wrong button color and should match the create-action styling used elsewhere. Update the q-btn inside the menu creation form to use the positive color instead of primary, following the same pattern as the create button in LeftNavEdit.vue and keeping the Create Menu action consistent with the sibling buttons in this dialog.Source: Coding guidelines
VueApp/src/CMS/__tests__/content-blocks.test.ts (1)
48-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
routeGetincontent-block-history.test.ts.Same pattern (mockGet.mockReset + implementation branching on URL substring) is repeated here almost verbatim. Consider extracting a shared factory into
test-utils.ts.🤖 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 `@VueApp/src/CMS/__tests__/content-blocks.test.ts` around lines 48 - 64, The routeGet mock setup in content-blocks.test.ts duplicates the same mockReset plus URL-branching implementation used in the content-block-history test. Extract this shared behavior into a reusable factory/helper in test-utils.ts, then update routeGet to delegate to it while preserving the /section-paths branch and pagination handling via mockGet, mockDel, and mockPost.VueApp/src/CMS/components/PersonSelector.vue (1)
58-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame unguarded
inject("apiURL")pattern asPermissionSelector.vue.Duplicate of the concern raised in
PermissionSelector.vue(Line 34): no default value,unknown-typed injection, silent malformed URL ifapiURLisn't provided by an ancestor.🤖 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 `@VueApp/src/CMS/components/PersonSelector.vue` at line 58, The apiURL injection in PersonSelector.vue is unguarded and can produce a malformed CMS options URL if no ancestor provides it. Update the PersonSelector setup logic around the apiURL/inject usage to mirror the safer pattern from PermissionSelector.vue: give inject("apiURL") a default or explicit guard, ensure the injected value is properly typed, and avoid constructing the URL unless a valid base URL is present.
🤖 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 `@VueApp/src/CMS/__tests__/content-block-edit-diff.test.ts`:
- Around line 105-166: These tests trigger Oxlint style warnings: add
expect.hasAssertions() in the async test cases, capitalize the describe/it
titles to match the project’s convention, prefer toStrictEqual where deep
equality is intended, and avoid top-level beforeEach hooks by moving the
routeGet setup into the relevant nested describe blocks in
content-block-edit-diff.test.ts.
In `@VueApp/src/CMS/__tests__/content-block-edit-save.test.ts`:
- Around line 152-160: The test file still has several oxlint-style warnings to
clean up, but the shared top-level beforeEach that resets mockGet, mockPost,
mockPut, mockDel, mockCommit, and calls routeGet() should stay in place because
it is intentionally applied across all describe blocks. Fix the remaining
stylistic issues in the surrounding tests to match the diff test file’s pattern,
and if require-top-level-describe is the only blocker here, suppress or ignore
that specific hint for this file rather than moving the shared setup.
In `@VueApp/src/CMS/__tests__/content-block-history.test.ts`:
- Around line 50-67: The mock routing logic in routeGet is duplicated across
content-block-history.test.ts and content-blocks.test.ts, so factor the shared
mock setup into a reusable routeGetFactory helper in test-utils.ts. Update the
tests to use that helper with mockGet so both suites share the same routing
behavior and stay in sync if the API handling changes.
In `@VueApp/src/CMS/__tests__/file-form-dialog-conflict.test.ts`:
- Around line 99-108: The managed-overwrite test coverage is incomplete around
the `existingModifiedOn`/`lastModifiedOn` contract in
`file-form-dialog-conflict.test.ts`. Update `NAME_TAKEN` to include
`existingModifiedOn` and strengthen the “PUTs to the existing record's guid”
assertion in the relevant `FileFormDialog`/`submit` flow test so it explicitly
checks whether `lastModifiedOn` is sent or omitted based on the backend
contract. Use the existing `NAME_TAKEN` fixture and the PUT request assertion
block to keep the guard covered.
In `@VueApp/src/CMS/__tests__/files.test.ts`:
- Around line 105-316: This test file has several oxlint warnings that may block
CI: add assertion-count checks to the affected it blocks, rename the describe
titles in Files.vue test suites to lowercase, and replace the non-strict
equality expectation with strict equality in the row presentation test. Also
remove or refactor the shared beforeEach/afterEach hooks flagged by no-hooks by
inlining setup/teardown into the individual tests or using local helper setup in
the relevant suites, and reduce the assertion count in the query param
construction test or split it into smaller tests to satisfy max-expects; use the
existing Files.vue suite names and test cases like query param construction,
delete and restore actions, row presentation, and URL and query watcher to
locate the changes.
In `@VueApp/src/CMS/__tests__/inline-file-upload.test.ts`:
- Around line 318-334: Add a test in inline-file-upload.test.ts around the
existing commitOf(wrapper) rollback scenario to cover when mockDel rejects
during the rollback loop in InlineFileUpload.vue. Use the same staged upload
setup, but make the first rollback delete fail and assert that commitOf(wrapper)
still rejects with the original upload error from postForm while the failure in
del() does not silently stop the remaining rollback behavior. Reference the
commitOf, mockDel, and mockPostForm flows to keep the new case aligned with the
existing batch rollback test.
In `@VueApp/src/CMS/__tests__/left-nav-edit.test.ts`:
- Around line 49-55: The test setup in left-nav-edit.test.ts is using a hook
pattern that conflicts with the project’s no-hooks lint rule, and the mocked GET
can be simplified. Update the test initialization around the beforeEach block
and mockGet so the setup avoids a disallowed hook pattern, and switch the menu
fetch stub in mockGet to a resolved-value style helper instead of an explicit
Promise wrapper.
In `@VueApp/src/CMS/__tests__/left-nav-menu-dialog.test.ts`:
- Around line 54-74: The test in left-nav-menu-dialog.test.ts has too many
assertions in one case, so split the current “POSTs the menu and emits created +
close on success” test into two focused tests. Keep one test around the POST
request and payload normalization using mockPost, mountDialog, and
submitDialogForm, and move the emitted event checks for created and
update:modelValue into a separate success test. This will bring each test under
the max-expects limit while preserving the existing behavior coverage.
In `@VueApp/src/CMS/components/ContentBlock.vue`:
- Around line 23-33: The ContentBlock loading flow has a stale-response race: an
earlier get() call in loadContentBlock can finish after a later
props.contentBlockName change and overwrite contentBlock.value with outdated
data. Update loadContentBlock and the watch on props.contentBlockName to ignore
out-of-order results by tracking the latest requested block name or a request
token, and only assigning to contentBlock.value when the response still matches
the current props.contentBlockName.
In `@VueApp/src/CMS/components/FileFormDialog.vue`:
- Around line 36-49: The dialog header and copy-link logic in FileFormDialog are
still reading stale props.file data after reloadLatest() updates latestFile.
Update the template bindings for the displayed friendlyName/friendlyUrl and the
copyUrl() path to use the refreshed latestFile (or the form state populated from
it) instead of props.file, so the shown and copied link always match the latest
record after a 409 refresh.
In `@VueApp/src/CMS/components/InlineFileUpload.vue`:
- Around line 109-115: The local NameCheck type in InlineFileUpload.vue has
drifted from the same shape used by FileFormDialog.vue and is missing
existingModifiedOn. Update the shared NameCheck definition by moving it into
VueApp/src/CMS/types/index.ts and have both InlineFileUpload.vue and
FileFormDialog.vue import that single type so the check-name response shape
stays consistent.
- Around line 3-11: The drop-zone/click target in InlineFileUpload should use
Quasar’s q-btn instead of a raw button element. Update the template to replace
the native button with q-btn while preserving the existing classes, disabled
state, aria-label, and click behavior, and adjust the useDropZone target to use
dropZoneRef.value?.$el so the drop zone still binds to the rendered component
element.
- Around line 288-311: The inline upload styles use fixed px values for the
border and hover ring, which should follow the stylesheet convention of using
rem units instead. Update the border thickness in InlineFileUpload.vue’s upload
state styles and the box-shadow spread in the .inline-upload--over rule to
rem-based values, keeping the existing visual appearance consistent with the
.inline-upload:focus-visible styling.
- Around line 198-219: The rollback path in commit() is not resilient because a
failing del(apiURL + guid) can abort the cleanup loop and mask the original
error from commitOne(). Update the catch block in InlineFileUpload.vue’s
commit() so each createdGuids rollback is attempted independently, with per-guid
failure handling that continues cleanup for the remaining GUIDs and
preserves/rethrows the original caught error e rather than a rollback error.
In `@VueApp/src/CMS/components/ModifiedStamp.vue`:
- Around line 30-38: The date formatting in ModifiedStamp.vue is hardcoded to
en-US, so verify whether the app should use the browser/user locale or a shared
date-formatting helper instead of a fixed locale. Update the computed formatted
value in the ModifiedStamp component to match the app’s existing date-stamp
convention, ideally by reusing the same formatting utility or locale strategy
used elsewhere in the CMS rather than embedding the locale directly.
In `@VueApp/src/CMS/components/PersonSelector.vue`:
- Around line 11-13: QSelect in PersonSelector.vue is using object options
without a label field, so its internal label resolution is incomplete for
navigation and accessibility. Update the QSelect configuration in the
PersonSelector component to explicitly set option-label to a real field from
CmsPersonOption (such as name) alongside the existing option-value="iamId", and
make sure the same mapping is applied anywhere else in the component where the
options are rendered or reused (including the selected-item/option slot setup).
In `@VueApp/src/CMS/pages/BulkEncrypt.vue`:
- Around line 224-258: The confirmation dialog logic in encryptSelected
duplicates the same Promise-based wrapper used by confirmAction in Files.vue, so
factor it into a shared useConfirmDialog composable. Move the $q.dialog
onOk/onCancel/onDismiss resolution pattern into the reusable helper, then update
encryptSelected to call that helper with the bulk-encrypt title/message/button
labels while preserving the current confirmed flow and post/encrypting handling.
In `@VueApp/src/CMS/pages/ContentBlockEdit.vue`:
- Around line 471-485: The loadHistoryVersion flow in ContentBlockEdit.vue
currently only handles the success path, so failed history loads fail silently.
Update loadHistoryVersion (and the related restoreBlock path mentioned in the
same component) to handle the non-success case from get(...) by showing a
$q.notify error message, matching the failure handling used by
diffAgainstCurrent and the restoreBlock implementation in ContentBlocks.vue.
Keep the existing success behavior intact, but add explicit user feedback
whenever res.success is false or the request fails.
In `@VueApp/src/CMS/pages/FileAuditLog.vue`:
- Around line 177-187: The action-to-color mapping in getActionColor is
duplicated with AuditList.getActionColor, so extract it into a shared utility
and have both audit views call the same helper. Create a reusable shared
function/composable (for example, a shared audit-colors module) that contains
the single source of truth for
AccessFileDenied/Add/Upload/Edit/Delete/CancelDelete/Import mappings, then
update FileAuditLog.vue and the Effort audit trail to import and use it.
In `@VueApp/src/CMS/pages/Files.vue`:
- Around line 312-318: The route.query to filters mapping is duplicated between
the initial filters ref and the route watcher, so extract a shared helper that
parses folder, status, search, encryptedOnly, and publicOnly into a single
object. Reuse that helper from both the filters initialization and the
route.query watcher to keep behavior consistent and avoid drift when adding or
changing filter fields. Locate the existing logic in the filters ref setup and
the route watcher around the route query handling.
- Around line 437-477: The confirm dialog logic is duplicated in confirmAction
and should be shared instead of being reimplemented inline in encryptSelected.
Extract the Promise-wrapped $q.dialog pattern from confirmAction into a reusable
composable such as useConfirmDialog under src/composables, then update both
Files.vue and BulkEncrypt.vue to call that shared helper for consistent dialog
behavior and easier reuse.
In `@VueApp/src/CMS/pages/LeftNavEdit.vue`:
- Around line 559-591: The scoped styles in LeftNavEdit.vue mix px and rem
units, so update the `@media (width >= 1024px)` breakpoint and the `border: 1px
solid` / `border-bottom: 1px solid` declarations in the `.menu-items
:deep(.sortable-row)` rules to use rem-based values instead. Keep the existing
selectors (`.menu-items`, `.sortable-row`, `.sortable-row:last-child`,
`.sortable-row.menu-item--header`) unchanged and align the sizing with the
nearby rem-based container query.
- Around line 35-50: The submit button in LeftNavEdit.vue is using the generic
primary color even when it renders the create flow label, so align its styling
with the create-action convention used by the Add Header/Add Link controls or
make the intent explicit if it is meant to stay a generic form submit. Update
the q-btn in the menu form (and any shared create-action button logic around the
Add Header/Add Link controls) so the label/state mapping in the same component
consistently uses positive for creation actions and primary only for non-create
submits.
- Around line 249-274: The empty menu object shape is duplicated in
LeftNavEdit.vue and LeftNavMenuDialog.vue, which makes the defaults hard to keep
in sync. Extract the repeated default menu literal into a shared factory or
helper and reuse it both where menu is initialized in LeftNavEdit.vue and where
the watch(menuId, ...) reset recreates the menu, then import the same helper
into LeftNavMenuDialog.vue so all three spots use one source of truth.
In `@VueApp/src/CMS/pages/ManageLinkCollections.vue`:
- Around line 428-462: The collection dialog’s unsaved-changes snapshot in
collectionDialogState is missing the pending New tag category input, so typing
into addTag can be lost without confirmCollectionClose() noticing. Update the
computed state used by useUnsavedChanges to include the live addTag value
alongside name, draftTags, and deletedTagIds, and keep the existing
deep-tracking approach so the guard reflects in-progress tag entry as well as
staged tag edits. This change belongs in the ManageLinkCollections.vue dialog
state and should align with the existing link dialog baseline/guard pattern.
In `@VueApp/src/components/__tests__/sortable-list.test.ts`:
- Around line 74-109: The SortableList move-buttons tests are missing coverage
for the disabled state, so add a case in the existing sortable-list.test.ts
suite using mountList and threeItems with disabled: true. Assert that the Move
up/Move down controls in the SortableList.vue behavior are non-interactive when
disabled is passed, and that clicking them does not reorder rows or emit
reorder. Keep the new assertions alongside the current “move buttons” tests and
reference the existing aria-label selectors used there.
In `@VueApp/src/components/SortableList.vue`:
- Around line 18-26: The drag path in SortableList.vue still hardcodes
VueDraggable animation, so it ignores the user’s reduced-motion preference even
though the button move path already checks reducedMotion.value. Update the
VueDraggable binding in SortableList to use the same reduced-motion condition as
the existing move logic, and ensure the drag animation is disabled or reduced
when reducedMotion.value indicates "reduce". Use the existing reducedMotion and
onDragEnd-related logic in SortableList.vue as the reference points.
- Around line 50-76: The move controls in SortableList.vue are not respecting
the existing disabled state, so users can still reorder via the up/down buttons
while VueDraggable is locked. Update the q-btn handlers and disable bindings in
the SortableList component so the buttons are also gated by the disabled prop,
using the existing disabled value alongside the index checks in onMoveUp and
onMoveDown. Make sure the same disabled condition is applied to the button state
and interaction path so reordering cannot happen mid-save.
In `@VueApp/src/composables/ViperFetch.ts`:
- Around line 274-288: The postForm and putForm signatures in ViperFetch violate
the default-param-last rule because the optional url argument is defaulted
before the required body argument. Update these two functions to remove the
unused default from url (or otherwise make the required parameters come first)
and keep their call sites and fetchWrapper usage unchanged; use the postForm and
putForm identifiers to locate the affected methods.
In `@VueApp/src/shared/__tests__/create-spa-router.test.ts`:
- Around line 13-36: Add a test case in createSpaRouter scroll behavior coverage
for the savedPosition branch: when router.options.scrollBehavior is called with
a non-null savedPosition, it should return that saved position instead of
applying the normal route/hash logic. Use the existing route helper and
scrollBehavior variable in create-spa-router.test.ts so the new assertion covers
the popstate/back-forward navigation path.
In `@VueApp/src/styles/base.css`:
- Around line 85-91: The focus style on .q-card__actions .q-btn:focus-visible in
base.css relies only on box-shadow and hardcodes the ring color, so update that
rule to use the shared --focus-ring-color token from colors.css instead of
repeating the hex value and add a transparent outline fallback alongside the
existing shadow. Keep the same selector and preserve the current visual
appearance while making the focus indicator resilient in forced-colors mode.
---
Outside diff comments:
In `@VueApp/src/CMS/components/LinkCollections.vue`:
- Around line 16-22: Add a short inline comment near the q-expansion-item
header-class in LinkCollections.vue explaining that lt-sm intentionally hides
the header on sm+ screens because filtersExpandedComputed forces the panel open
for $q.screen.gt.xs. Reference filtersExpandedComputed and the q-expansion-item
so future edits don’t remove this coupling or “fix” the hidden desktop header.
In `@VueApp/src/CMS/pages/ManageLinkCollections.vue`:
- Around line 512-563: The partial tag-save recovery in handleTagFailure reloads
server tags and resets draftTags/deletedTagIds, but it leaves the original
baseline stale. Update handleTagFailure in ManageLinkCollections.vue to also
re-baseline the dialog state after loadTags() by calling setCollectionBaseline()
so the refreshed draft state matches the true server state. Keep this in the
same recovery path used by the tag mutation loop (delete/add/order failures) so
closing the dialog does not trigger a false Unsaved Changes warning.
In `@VueApp/src/Effort/router/index.ts`:
- Around line 13-52: The eval-permission refresh in router.beforeEach is
currently tied to the non-internal navigation path, so a failed load can
permanently prevent future retries for the session. Move the SVMSecure.Eval
permission check/load logic out of the isInternalNavigation gate so it still
runs when needed on later navigations, and update loadEvalPermissions() to catch
errors from useFetch().get() and log them instead of letting rejections escape.
Keep the retry latch reset in loadEvalPermissions() so future attempts can
refetch after a failure.
---
Duplicate comments:
In `@VueApp/src/CMS/__tests__/content-blocks.test.ts`:
- Around line 48-64: The routeGet mock setup in content-blocks.test.ts
duplicates the same mockReset plus URL-branching implementation used in the
content-block-history test. Extract this shared behavior into a reusable
factory/helper in test-utils.ts, then update routeGet to delegate to it while
preserving the /section-paths branch and pagination handling via mockGet,
mockDel, and mockPost.
In `@VueApp/src/CMS/components/LeftNavMenuDialog.vue`:
- Around line 53-60: The Create Menu action in LeftNavMenuDialog.vue uses the
wrong button color and should match the create-action styling used elsewhere.
Update the q-btn inside the menu creation form to use the positive color instead
of primary, following the same pattern as the create button in LeftNavEdit.vue
and keeping the Create Menu action consistent with the sibling buttons in this
dialog.
In `@VueApp/src/CMS/components/PersonSelector.vue`:
- Line 58: The apiURL injection in PersonSelector.vue is unguarded and can
produce a malformed CMS options URL if no ancestor provides it. Update the
PersonSelector setup logic around the apiURL/inject usage to mirror the safer
pattern from PermissionSelector.vue: give inject("apiURL") a default or explicit
guard, ensure the injected value is properly typed, and avoid constructing the
URL unless a valid base URL is present.
🪄 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: e20f3196-1912-4855-926f-c0fc4950dfbf
📒 Files selected for processing (86)
VueApp/src/CAHFS/router/index.tsVueApp/src/CMS/__tests__/cms-home.test.tsVueApp/src/CMS/__tests__/content-block-edit-diff.test.tsVueApp/src/CMS/__tests__/content-block-edit-save.test.tsVueApp/src/CMS/__tests__/content-block-history.test.tsVueApp/src/CMS/__tests__/content-blocks.test.tsVueApp/src/CMS/__tests__/content-diff-dialog.test.tsVueApp/src/CMS/__tests__/date-range-filter.test.tsVueApp/src/CMS/__tests__/file-form-dialog-conflict.test.tsVueApp/src/CMS/__tests__/file-form-dialog.test.tsVueApp/src/CMS/__tests__/files.test.tsVueApp/src/CMS/__tests__/inline-file-upload.test.tsVueApp/src/CMS/__tests__/left-nav-edit.test.tsVueApp/src/CMS/__tests__/left-nav-menu-dialog.test.tsVueApp/src/CMS/__tests__/left-nav-menus.test.tsVueApp/src/CMS/__tests__/link-collections.test.tsVueApp/src/CMS/__tests__/link.test.tsVueApp/src/CMS/__tests__/modified-stamp.test.tsVueApp/src/CMS/__tests__/permission-selector.test.tsVueApp/src/CMS/__tests__/person-selector.test.tsVueApp/src/CMS/__tests__/recent-activity.test.tsVueApp/src/CMS/__tests__/router-canonicalization.test.tsVueApp/src/CMS/__tests__/test-utils.tsVueApp/src/CMS/__tests__/url.test.tsVueApp/src/CMS/__tests__/use-server-table.test.tsVueApp/src/CMS/__tests__/use-url-filtered-table.test.tsVueApp/src/CMS/components/ActivityRow.vueVueApp/src/CMS/components/ContentBlock.vueVueApp/src/CMS/components/ContentDiffDialog.vueVueApp/src/CMS/components/DateRangeFilter.vueVueApp/src/CMS/components/DeleteRestoreButtons.vueVueApp/src/CMS/components/EditButton.vueVueApp/src/CMS/components/FileFormDialog.vueVueApp/src/CMS/components/InlineFileUpload.vueVueApp/src/CMS/components/LeftNavMenuDialog.vueVueApp/src/CMS/components/LeftNavMenuSettingsFields.vueVueApp/src/CMS/components/Link.vueVueApp/src/CMS/components/LinkCollections.vueVueApp/src/CMS/components/ListCard.vueVueApp/src/CMS/components/ListCardField.vueVueApp/src/CMS/components/ModifiedStamp.vueVueApp/src/CMS/components/PermissionChips.vueVueApp/src/CMS/components/PermissionSelector.vueVueApp/src/CMS/components/PersonSelector.vueVueApp/src/CMS/components/RecentActivity.vueVueApp/src/CMS/components/StatusIcon.vueVueApp/src/CMS/composables/use-content-diff-viewer.tsVueApp/src/CMS/composables/use-server-table.tsVueApp/src/CMS/composables/use-url-filtered-table.tsVueApp/src/CMS/file-types.tsVueApp/src/CMS/pages/BulkEncrypt.vueVueApp/src/CMS/pages/CmsHome.vueVueApp/src/CMS/pages/ContentBlockEdit.vueVueApp/src/CMS/pages/ContentBlockHistory.vueVueApp/src/CMS/pages/ContentBlocks.vueVueApp/src/CMS/pages/FileAuditLog.vueVueApp/src/CMS/pages/Files.vueVueApp/src/CMS/pages/ImportFiles.vueVueApp/src/CMS/pages/LeftNavEdit.vueVueApp/src/CMS/pages/LeftNavMenus.vueVueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CMS/router/index.tsVueApp/src/CMS/router/routes.tsVueApp/src/CMS/types/index.tsVueApp/src/CTS/router/index.tsVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/EffortLeftNav.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/router/index.tsVueApp/src/Students/router/index.tsVueApp/src/components/BreadcrumbHeading.vueVueApp/src/components/SortableList.vueVueApp/src/components/__tests__/sortable-list.test.tsVueApp/src/composables/ViperFetch.tsVueApp/src/composables/__tests__/use-reorder.test.tsVueApp/src/composables/use-reorder.tsVueApp/src/composables/use-route-focus.tsVueApp/src/config/colors.tsVueApp/src/layouts/LeftNav.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/shared/__tests__/create-spa-router.test.tsVueApp/src/shared/create-spa-router.tsVueApp/src/shared/createSpaRouter.tsVueApp/src/styles/base.cssVueApp/src/styles/colors.css
💤 Files with no reviewable changes (1)
- VueApp/src/shared/createSpaRouter.ts
Contributor
✅ Action performedReview finished.
|
rlorenzo
added a commit
that referenced
this pull request
Jul 3, 2026
- extract getAuditActionColor to a shared composable so the CMS file audit and the Effort audit list (and any future audit surface) color the same verb classes identically, per PR #253 review feedback - extract ChangeDetailDiff from AuditList's three layouts to remove the old/new diff markup duplication flagged by the JSCPD gate
rlorenzo
force-pushed
the
VPR-59-cms-r3-frontend
branch
from
July 23, 2026 16:12
e174eb7 to
dae1ade
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VueApp/src/CTS/pages/ManageEpas.vue (1)
108-120: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winIdentical
RichTextEditortoolbar array duplicated across two CTS pages.Both files hand-roll the same five-group toolbar (
left/center/right/justify,bold/italic/underline/strike,quote/unordered/ordered/outdent/indent,undo/redo,viewsource). Extract it to a shared constant (e.g.VueApp/src/CTS/constants.ts) so both editors stay in sync when the toolbar changes.
VueApp/src/CTS/pages/ManageEpas.vue#L108-L120: import and use the shared toolbar constant instead of the inline array at lines 113-119.VueApp/src/CTS/pages/ManageMilestones.vue#L167-L178: import and use the same shared toolbar constant instead of the inline array at lines 171-177.♻️ Proposed fix
// VueApp/src/CTS/constants.ts export const CTS_RICH_TEXT_TOOLBAR = [ ["left", "center", "right", "justify"], ["bold", "italic", "underline", "strike"], ["quote", "unordered", "ordered", "outdent", "indent"], ["undo", "redo"], ["viewsource"], ]-import RichTextEditor from "`@/components/RichTextEditor.vue`" +import RichTextEditor from "`@/components/RichTextEditor.vue`" +import { CTS_RICH_TEXT_TOOLBAR } from "`@/CTS/constants`"- <RichTextEditor - v-model="epa.description" - min-height="15rem" - class="col col-lg-6" - aria-label="EPA description" - :toolbar="[ - ['left', 'center', 'right', 'justify'], - ['bold', 'italic', 'underline', 'strike'], - ['quote', 'unordered', 'ordered', 'outdent', 'indent'], - ['undo', 'redo'], - ['viewsource'], - ]" - /> + <RichTextEditor + v-model="epa.description" + min-height="15rem" + class="col col-lg-6" + aria-label="EPA description" + :toolbar="CTS_RICH_TEXT_TOOLBAR" + />🤖 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 `@VueApp/src/CTS/pages/ManageEpas.vue` around lines 108 - 120, Extract the duplicated RichTextEditor toolbar into a shared CTS constant, such as CTS_RICH_TEXT_TOOLBAR in VueApp/src/CTS/constants.ts, preserving all five groups and their order. In VueApp/src/CTS/pages/ManageEpas.vue lines 108-120 and VueApp/src/CTS/pages/ManageMilestones.vue lines 167-178, import and bind this constant instead of defining inline toolbar arrays.
♻️ Duplicate comments (2)
VueApp/src/CMS/components/PermissionSelector.vue (2)
34-49: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract a shared CMS options service instead of duplicating apiURL/fetch logic. Both selectors inline-build
apiURLviainject("apiURL") + "cms/options/"and calluseFetch().get(...)directly, duplicating the same request pattern against sibling endpoints.
VueApp/src/CMS/components/PermissionSelector.vue#L34-L49: moveloadPermissions's URL-building anduseFetch().get(apiURL + "permissions")call into a shared CMS options service function.VueApp/src/CMS/components/PersonSelector.vue#L60-L86: movesearchPeople's URL-building anduseFetch().get(...)call into the same shared service (e.g. agetPermissionOptions()/searchPeopleOptions()pair in one module), keeping each component's own loading/caching/out-of-order logic.As per coding guidelines: "Frontend API calls must use a service layer and
useFetch(), never rawfetch(), and must unwrap{ result, success }." and "When fixing a bug, verify every affected code path, check duplicate or parallel implementations, and fix consistently or extract shared logic."🤖 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 `@VueApp/src/CMS/components/PermissionSelector.vue` around lines 34 - 49, Extract the duplicated CMS options request logic from PermissionSelector.loadPermissions and PersonSelector.searchPeople into one shared service module, with separate functions for permission options and people searches. Each service function must build the injected API base and CMS options path, call useFetch().get, and return the unwrapped result while preserving success handling. Update both components to use the service, retaining their existing loading, caching, and out-of-order response logic; apply changes in VueApp/src/CMS/components/PermissionSelector.vue lines 34-49 and VueApp/src/CMS/components/PersonSelector.vue lines 60-86.Source: Coding guidelines
34-49: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStill bypassing the service layer for permission options.
apiURLis built inline viainject("apiURL")anduseFetch().get(...)is called directly in the component, rather than through a CMS options service. This was flagged previously and remains unresolved.PersonSelector.vuerepeats the exact same pattern for a different endpoint — see consolidated comment.As per coding guidelines: "Frontend API calls must use a service layer and
useFetch(), never rawfetch(), and must unwrap{ result, success }."🤖 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 `@VueApp/src/CMS/components/PermissionSelector.vue` around lines 34 - 49, Update loadPermissions in PermissionSelector.vue to obtain permission options through the existing CMS options service instead of constructing apiURL with inject("apiURL") and calling useFetch().get directly. Reuse the service-layer method for the permissions endpoint, preserve the success/result unwrapping into allPermissions and the existing loading state behavior, and apply the same service pattern used by the corresponding selector implementation.Source: Coding guidelines
🤖 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 `@VueApp/src/CMS/components/ActivityRow.vue`:
- Around line 64-66: Update formatFullDate to use the CMS formatter from
useDateFunctions().formatDateTime() instead of Date.toLocaleString(), preserving
the fixed en-US CMS date format for the tooltip.
In `@VueApp/src/CMS/components/DateRangeFilter.vue`:
- Around line 30-31: Update the DateRangeFilter models from, to, and their
parent filter value types to accept string | null, matching clearable date input
behavior. In date-range-filter.test.ts, add coverage for clearing each field,
verifying the corresponding update:from or update:to emission and change event.
In `@VueApp/src/CMS/components/ListCard.vue`:
- Around line 2-19: Replace the outer div in ListCard.vue with an appropriate
Quasar component and transfer its col-12 q-pa-xs classes to that component,
preserving the existing card structure. In ModifiedStamp.vue, replace the raw
span wrappers in both rendering branches with suitable Quasar text/layout
components while preserving their content and behavior. Apply these changes at
VueApp/src/CMS/components/ListCard.vue lines 2-19 and
VueApp/src/CMS/components/ModifiedStamp.vue lines 7-30.
In `@VueApp/src/CMS/components/PermissionChips.vue`:
- Around line 13-22: The q-chip’s accessible name does not match its visible “+N
more” label. Update the aria-label binding in the PermissionChips template to
include the same visible count-and-more wording, while retaining the permission
count context needed for accessibility.
In `@VueApp/src/CMS/pages/ContentBlockHistory.vue`:
- Around line 316-321: Update the .diff-action-btn rules within the
coarse-pointer media query to use 2.75rem instead of 44px for both min-width and
min-height, preserving the existing touch-target dimensions and stylesheet rem
convention.
In `@VueApp/src/Effort/components/EffortLeftNav.vue`:
- Line 16: Update the QItem elements in EffortLeftNav so every routed or anchor
item has role="link", while noninteractive header items have
role="presentation"; preserve the existing navigation behavior and apply the
roles consistently across the parallel navigation.
---
Outside diff comments:
In `@VueApp/src/CTS/pages/ManageEpas.vue`:
- Around line 108-120: Extract the duplicated RichTextEditor toolbar into a
shared CTS constant, such as CTS_RICH_TEXT_TOOLBAR in
VueApp/src/CTS/constants.ts, preserving all five groups and their order. In
VueApp/src/CTS/pages/ManageEpas.vue lines 108-120 and
VueApp/src/CTS/pages/ManageMilestones.vue lines 167-178, import and bind this
constant instead of defining inline toolbar arrays.
---
Duplicate comments:
In `@VueApp/src/CMS/components/PermissionSelector.vue`:
- Around line 34-49: Extract the duplicated CMS options request logic from
PermissionSelector.loadPermissions and PersonSelector.searchPeople into one
shared service module, with separate functions for permission options and people
searches. Each service function must build the injected API base and CMS options
path, call useFetch().get, and return the unwrapped result while preserving
success handling. Update both components to use the service, retaining their
existing loading, caching, and out-of-order response logic; apply changes in
VueApp/src/CMS/components/PermissionSelector.vue lines 34-49 and
VueApp/src/CMS/components/PersonSelector.vue lines 60-86.
- Around line 34-49: Update loadPermissions in PermissionSelector.vue to obtain
permission options through the existing CMS options service instead of
constructing apiURL with inject("apiURL") and calling useFetch().get directly.
Reuse the service-layer method for the permissions endpoint, preserve the
success/result unwrapping into allPermissions and the existing loading state
behavior, and apply the same service pattern used by the corresponding selector
implementation.
🪄 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 Plus
Run ID: d6439f68-a5cb-4ca6-9d9d-856deef7c91a
📒 Files selected for processing (98)
DESIGN.mdVueApp/src/CAHFS/router/index.tsVueApp/src/CMS/__tests__/cms-home.test.tsVueApp/src/CMS/__tests__/content-block-edit-diff.test.tsVueApp/src/CMS/__tests__/content-block-edit-save.test.tsVueApp/src/CMS/__tests__/content-block-history.test.tsVueApp/src/CMS/__tests__/content-blocks.test.tsVueApp/src/CMS/__tests__/content-diff-dialog.test.tsVueApp/src/CMS/__tests__/date-range-filter.test.tsVueApp/src/CMS/__tests__/file-form-dialog-conflict.test.tsVueApp/src/CMS/__tests__/file-form-dialog.test.tsVueApp/src/CMS/__tests__/files.test.tsVueApp/src/CMS/__tests__/import-files.test.tsVueApp/src/CMS/__tests__/inline-file-upload.test.tsVueApp/src/CMS/__tests__/left-nav-edit.test.tsVueApp/src/CMS/__tests__/left-nav-menu-dialog.test.tsVueApp/src/CMS/__tests__/left-nav-menus.test.tsVueApp/src/CMS/__tests__/link-collections.test.tsVueApp/src/CMS/__tests__/link.test.tsVueApp/src/CMS/__tests__/modified-stamp.test.tsVueApp/src/CMS/__tests__/permission-selector.test.tsVueApp/src/CMS/__tests__/person-selector.test.tsVueApp/src/CMS/__tests__/recent-activity.test.tsVueApp/src/CMS/__tests__/router-canonicalization.test.tsVueApp/src/CMS/__tests__/test-utils.tsVueApp/src/CMS/__tests__/url.test.tsVueApp/src/CMS/__tests__/use-server-table.test.tsVueApp/src/CMS/__tests__/use-url-filtered-table.test.tsVueApp/src/CMS/components/ActivityRow.vueVueApp/src/CMS/components/ContentBlock.vueVueApp/src/CMS/components/ContentDiffDialog.vueVueApp/src/CMS/components/DateRangeFilter.vueVueApp/src/CMS/components/DeleteRestoreButtons.vueVueApp/src/CMS/components/EditButton.vueVueApp/src/CMS/components/FileFormDialog.vueVueApp/src/CMS/components/InlineFileUpload.vueVueApp/src/CMS/components/LeftNavMenuDialog.vueVueApp/src/CMS/components/LeftNavMenuSettingsFields.vueVueApp/src/CMS/components/Link.vueVueApp/src/CMS/components/LinkCollections.vueVueApp/src/CMS/components/ListCard.vueVueApp/src/CMS/components/ListCardField.vueVueApp/src/CMS/components/ModifiedStamp.vueVueApp/src/CMS/components/PermissionChips.vueVueApp/src/CMS/components/PermissionSelector.vueVueApp/src/CMS/components/PersonSelector.vueVueApp/src/CMS/components/RecentActivity.vueVueApp/src/CMS/components/StatusIcon.vueVueApp/src/CMS/composables/use-content-diff-viewer.tsVueApp/src/CMS/composables/use-server-table.tsVueApp/src/CMS/composables/use-url-filtered-table.tsVueApp/src/CMS/file-types.tsVueApp/src/CMS/pages/BulkEncrypt.vueVueApp/src/CMS/pages/CmsHome.vueVueApp/src/CMS/pages/ContentBlockEdit.vueVueApp/src/CMS/pages/ContentBlockHistory.vueVueApp/src/CMS/pages/ContentBlocks.vueVueApp/src/CMS/pages/FileAuditLog.vueVueApp/src/CMS/pages/Files.vueVueApp/src/CMS/pages/ImportFiles.vueVueApp/src/CMS/pages/LeftNavEdit.vueVueApp/src/CMS/pages/LeftNavMenus.vueVueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CMS/router/index.tsVueApp/src/CMS/router/routes.tsVueApp/src/CMS/types/index.tsVueApp/src/CTS/pages/ManageEpas.vueVueApp/src/CTS/pages/ManageMilestones.vueVueApp/src/CTS/router/index.tsVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/EffortLeftNav.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/router/index.tsVueApp/src/Students/router/index.tsVueApp/src/components/BreadcrumbHeading.vueVueApp/src/components/RichTextEditor.vueVueApp/src/components/SortableList.vueVueApp/src/components/__tests__/rich-text-editor.test.tsVueApp/src/components/__tests__/sortable-list.test.tsVueApp/src/composables/RequireLogin.tsVueApp/src/composables/ViperFetch.tsVueApp/src/composables/__tests__/use-audit-colors.test.tsVueApp/src/composables/__tests__/use-reorder.test.tsVueApp/src/composables/use-audit-colors.tsVueApp/src/composables/use-reorder.tsVueApp/src/composables/use-route-focus.tsVueApp/src/config/colors.tsVueApp/src/layouts/LeftNav.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/shared/__tests__/create-spa-router.test.tsVueApp/src/shared/__tests__/strip-trailing-slashes.test.tsVueApp/src/shared/bootstrap-spa.tsVueApp/src/shared/create-spa-router.tsVueApp/src/shared/createSpaRouter.tsVueApp/src/shared/strip-trailing-slashes.tsVueApp/src/styles/base.cssVueApp/src/styles/colors.css
💤 Files with no reviewable changes (1)
- VueApp/src/shared/createSpaRouter.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 100 out of 100 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
VueApp/src/composables/use-reorder.ts:6
- DEFAULT_FLASH_MS is 600ms, but SortableList’s flash animation runs for 1s ("animation: sortable-row-flash 1s ease-out"). Since justMovedKey is cleared after flashMs, the "sortable-row--moved" class drops ~400ms before the CSS animation completes, cutting the visual cue short. Align the default flash duration with the CSS animation duration (or update the CSS to match).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
VueApp/src/CMS/components/Link.vue (1)
26-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNested per-item scan over tag lists.
The inner
v-forrescans everylink.linkTagsfor each category, an O(categories × tags) scan per card. Pre-build aMap<categoryId, tags[]>(computed) and look up per category in the loop. Also use strict===at Line 35.As per coding guidelines: "Avoid nested per-item searches over growable lists; pre-build a
Map,Set, orDictionaryand look up items in the loop."🤖 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 `@VueApp/src/CMS/components/Link.vue` around lines 26 - 42, In the Link component’s tag-rendering template, replace the nested category-to-tag scan with a computed Map keyed by linkCollectionTagCategoryId, grouping props.link.linkTags into tag arrays and looking up each category’s tags during the outer loop. Render only the looked-up tags, and change the category comparison in StatusBadge’s v-if to strict ===.Source: Coding guidelines
VueApp/src/composables/RequireLogin.ts (1)
20-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild the login endpoint from the normalized base.
A valid
VITE_VIPER_HOME="/2"currently redirects to/2login, while"/2///"retains duplicate slashes. UseapplicationBasefor both login URLs.Proposed fix
function buildLoginUrl(returnPath: string): string { - const viperHome = import.meta.env.VITE_VIPER_HOME ?? "/" - const applicationBase = stripTrailingSlashes(viperHome) + const applicationBase = stripTrailingSlashes(import.meta.env.VITE_VIPER_HOME ?? "/") + const loginPath = `${applicationBase}/login` const fallbackPath = `${applicationBase}/` if (isValidInternalPath(returnPath)) { - return `${viperHome}login?ReturnUrl=${encodeURIComponent(returnPath)}` + return `${loginPath}?ReturnUrl=${encodeURIComponent(returnPath)}` } - return `${viperHome}login?ReturnUrl=${encodeURIComponent(fallbackPath)}` + return `${loginPath}?ReturnUrl=${encodeURIComponent(fallbackPath)}` }🤖 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 `@VueApp/src/composables/RequireLogin.ts` around lines 20 - 28, Update buildLoginUrl to construct both login URLs from the normalized applicationBase rather than the raw viperHome value, ensuring VITE_VIPER_HOME values such as "/2" and "/2///" produce a correctly separated, slash-normalized login endpoint while preserving the existing return-path selection.
🤖 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 `@VueApp/src/CMS/__tests__/content-blocks.test.ts`:
- Around line 203-207: Replace the duplicated local clickBodyButton helper with
the shared export from ./test-utils. In
VueApp/src/CMS/__tests__/content-blocks.test.ts (lines 203-207),
VueApp/src/CMS/__tests__/files.test.ts (lines 99-103), and
VueApp/src/CMS/__tests__/left-nav-menus.test.ts (lines 69-73), delete each local
definition and add clickBodyButton to the corresponding ./test-utils import; no
other changes are needed.
In `@VueApp/src/CMS/components/LeftNavMenuDialog.vue`:
- Line 91: Update the apiURL injection in LeftNavMenuDialog so it is typed as a
string and defaults to the CMS /api/ base when no provider exists, then append
the existing cms/left-navs path for POST requests.
In `@VueApp/src/CMS/pages/BulkEncrypt.vue`:
- Around line 224-243: Update the BulkEncrypt.vue selection/encryption flow
around encryptSelected and the “Encrypt Selected” control to enforce the
controller’s 500-item limit: disable the action when selected.value.length
reaches 500, or split the selection into batches of at most 500 before POSTing
to bulk-encrypt. Ensure oversized selections cannot trigger a request exceeding
the limit.
---
Outside diff comments:
In `@VueApp/src/CMS/components/Link.vue`:
- Around line 26-42: In the Link component’s tag-rendering template, replace the
nested category-to-tag scan with a computed Map keyed by
linkCollectionTagCategoryId, grouping props.link.linkTags into tag arrays and
looking up each category’s tags during the outer loop. Render only the looked-up
tags, and change the category comparison in StatusBadge’s v-if to strict ===.
In `@VueApp/src/composables/RequireLogin.ts`:
- Around line 20-28: Update buildLoginUrl to construct both login URLs from the
normalized applicationBase rather than the raw viperHome value, ensuring
VITE_VIPER_HOME values such as "/2" and "/2///" produce a correctly separated,
slash-normalized login endpoint while preserving the existing return-path
selection.
🪄 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 Plus
Run ID: 8e57d807-6f04-456e-b242-f107aa36872b
📒 Files selected for processing (100)
DESIGN.mdVueApp/src/CAHFS/router/index.tsVueApp/src/CMS/__tests__/cms-home.test.tsVueApp/src/CMS/__tests__/content-block-edit-diff.test.tsVueApp/src/CMS/__tests__/content-block-edit-save.test.tsVueApp/src/CMS/__tests__/content-block-history.test.tsVueApp/src/CMS/__tests__/content-blocks.test.tsVueApp/src/CMS/__tests__/content-diff-dialog.test.tsVueApp/src/CMS/__tests__/date-range-filter.test.tsVueApp/src/CMS/__tests__/file-form-dialog-conflict.test.tsVueApp/src/CMS/__tests__/file-form-dialog.test.tsVueApp/src/CMS/__tests__/files.test.tsVueApp/src/CMS/__tests__/import-files.test.tsVueApp/src/CMS/__tests__/inline-file-upload.test.tsVueApp/src/CMS/__tests__/left-nav-edit.test.tsVueApp/src/CMS/__tests__/left-nav-menu-dialog.test.tsVueApp/src/CMS/__tests__/left-nav-menus.test.tsVueApp/src/CMS/__tests__/link-collections.test.tsVueApp/src/CMS/__tests__/link.test.tsVueApp/src/CMS/__tests__/modified-stamp.test.tsVueApp/src/CMS/__tests__/permission-selector.test.tsVueApp/src/CMS/__tests__/person-selector.test.tsVueApp/src/CMS/__tests__/recent-activity.test.tsVueApp/src/CMS/__tests__/router-canonicalization.test.tsVueApp/src/CMS/__tests__/test-utils.tsVueApp/src/CMS/__tests__/url.test.tsVueApp/src/CMS/__tests__/use-server-table.test.tsVueApp/src/CMS/__tests__/use-url-filtered-table.test.tsVueApp/src/CMS/components/ActivityRow.vueVueApp/src/CMS/components/ContentBlock.vueVueApp/src/CMS/components/ContentDiffDialog.vueVueApp/src/CMS/components/DateRangeFilter.vueVueApp/src/CMS/components/DeleteRestoreButtons.vueVueApp/src/CMS/components/EditButton.vueVueApp/src/CMS/components/FileFormDialog.vueVueApp/src/CMS/components/InlineFileUpload.vueVueApp/src/CMS/components/LeftNavMenuDialog.vueVueApp/src/CMS/components/LeftNavMenuSettingsFields.vueVueApp/src/CMS/components/Link.vueVueApp/src/CMS/components/LinkCollections.vueVueApp/src/CMS/components/ListCard.vueVueApp/src/CMS/components/ListCardField.vueVueApp/src/CMS/components/ModifiedStamp.vueVueApp/src/CMS/components/PermissionChips.vueVueApp/src/CMS/components/PermissionSelector.vueVueApp/src/CMS/components/PersonSelector.vueVueApp/src/CMS/components/RecentActivity.vueVueApp/src/CMS/components/StatusIcon.vueVueApp/src/CMS/composables/use-content-diff-viewer.tsVueApp/src/CMS/composables/use-server-table.tsVueApp/src/CMS/composables/use-url-filtered-table.tsVueApp/src/CMS/file-types.tsVueApp/src/CMS/pages/BulkEncrypt.vueVueApp/src/CMS/pages/CmsHome.vueVueApp/src/CMS/pages/ContentBlockEdit.vueVueApp/src/CMS/pages/ContentBlockHistory.vueVueApp/src/CMS/pages/ContentBlocks.vueVueApp/src/CMS/pages/FileAuditLog.vueVueApp/src/CMS/pages/Files.vueVueApp/src/CMS/pages/ImportFiles.vueVueApp/src/CMS/pages/LeftNavEdit.vueVueApp/src/CMS/pages/LeftNavMenus.vueVueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CMS/router/index.tsVueApp/src/CMS/router/routes.tsVueApp/src/CMS/services/cms-options-service.tsVueApp/src/CMS/types/index.tsVueApp/src/CTS/constants.tsVueApp/src/CTS/pages/ManageEpas.vueVueApp/src/CTS/pages/ManageMilestones.vueVueApp/src/CTS/router/index.tsVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/EffortLeftNav.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/router/index.tsVueApp/src/Students/router/index.tsVueApp/src/components/BreadcrumbHeading.vueVueApp/src/components/RichTextEditor.vueVueApp/src/components/SortableList.vueVueApp/src/components/__tests__/rich-text-editor.test.tsVueApp/src/components/__tests__/sortable-list.test.tsVueApp/src/composables/RequireLogin.tsVueApp/src/composables/ViperFetch.tsVueApp/src/composables/__tests__/use-audit-colors.test.tsVueApp/src/composables/__tests__/use-reorder.test.tsVueApp/src/composables/use-audit-colors.tsVueApp/src/composables/use-reorder.tsVueApp/src/composables/use-route-focus.tsVueApp/src/config/colors.tsVueApp/src/layouts/LeftNav.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/shared/__tests__/create-spa-router.test.tsVueApp/src/shared/__tests__/strip-trailing-slashes.test.tsVueApp/src/shared/bootstrap-spa.tsVueApp/src/shared/create-spa-router.tsVueApp/src/shared/createSpaRouter.tsVueApp/src/shared/strip-trailing-slashes.tsVueApp/src/styles/base.cssVueApp/src/styles/colors.css
💤 Files with no reviewable changes (1)
- VueApp/src/shared/createSpaRouter.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
Note: did similar UI improvements for the Files listing as the Content Block page |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@VueApp/src/CMS/__tests__/file-form-dialog.test.ts`:
- Around line 39-68: Extract the duplicated existingFile and mountDialog
fixtures into VueApp/src/CMS/__tests__/test-utils.ts, then remove the local
definitions and import the shared helpers in
VueApp/src/CMS/__tests__/file-form-dialog.test.ts (lines 39-68) and
VueApp/src/CMS/__tests__/file-form-dialog-conflict.test.ts (lines 43-72).
Preserve the existing helper behavior and update imports as needed.
In `@VueApp/src/CMS/components/ContentDiffDialog.vue`:
- Around line 145-153: Update the image outline declarations in the .cms-diff
selectors for ins.diffins, ins.diffmod, del.diffdel, and del.diffmod to use the
equivalent rem-based width instead of 2px, preserving the existing colors and
selector behavior.
In `@VueApp/src/CMS/composables/use-url-filtered-table.ts`:
- Around line 65-81: Update syncFiltersToUrl, reload, and clearPrimaryFilter to
return and propagate the promises from router.replace and table.reloadFirstPage,
matching the promise-returning behavior of use-server-table.ts so callers
awaiting reload or clearPrimaryFilter wait for URL synchronization and refetch
completion.
In `@VueApp/src/CMS/pages/BulkEncrypt.vue`:
- Line 224: Move both endpoint-specific calls in BulkEncrypt.vue—folder-count
retrieval and bulk encryption—from direct get/post usage into methods on a CMS
files service implemented with useFetch(). Update the page workflow to call
those service methods, preserving the existing request parameters and response
handling; apply the changes at VueApp/src/CMS/pages/BulkEncrypt.vue lines
224-224 and 249-252.
In `@VueApp/src/shared/__tests__/create-spa-router.test.ts`:
- Around line 45-127: Add a test in the “route-change focus management” suite
that configures a navigation guard to abort a route change, focuses an existing
element such as `#leftNavMenu`, attempts the blocked navigation, and verifies
focus remains there after the router settles. Exercise the failure early-return
in useRouteFocus while preserving the existing successful-navigation coverage.
In `@VueApp/src/shared/create-spa-router.ts`:
- Around line 21-32: Update the scrollBehavior callback to reset scroll when
navigation removes a hash, so transitioning from a hashed URL to the same path
without a hash returns the top-left position instead of false. Preserve
saved-position restoration and hash-target scrolling, and add a regression test
covering fragment removal.
🪄 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 Plus
Run ID: 85f398f6-1f5e-40df-a9b8-8b78db6c11a6
📒 Files selected for processing (100)
DESIGN.mdVueApp/src/CAHFS/router/index.tsVueApp/src/CMS/__tests__/cms-home.test.tsVueApp/src/CMS/__tests__/content-block-edit-diff.test.tsVueApp/src/CMS/__tests__/content-block-edit-save.test.tsVueApp/src/CMS/__tests__/content-block-history.test.tsVueApp/src/CMS/__tests__/content-blocks.test.tsVueApp/src/CMS/__tests__/content-diff-dialog.test.tsVueApp/src/CMS/__tests__/date-range-filter.test.tsVueApp/src/CMS/__tests__/file-form-dialog-conflict.test.tsVueApp/src/CMS/__tests__/file-form-dialog.test.tsVueApp/src/CMS/__tests__/files.test.tsVueApp/src/CMS/__tests__/import-files.test.tsVueApp/src/CMS/__tests__/inline-file-upload.test.tsVueApp/src/CMS/__tests__/left-nav-edit.test.tsVueApp/src/CMS/__tests__/left-nav-menu-dialog.test.tsVueApp/src/CMS/__tests__/left-nav-menus.test.tsVueApp/src/CMS/__tests__/link-collections.test.tsVueApp/src/CMS/__tests__/link.test.tsVueApp/src/CMS/__tests__/modified-stamp.test.tsVueApp/src/CMS/__tests__/permission-selector.test.tsVueApp/src/CMS/__tests__/person-selector.test.tsVueApp/src/CMS/__tests__/recent-activity.test.tsVueApp/src/CMS/__tests__/router-canonicalization.test.tsVueApp/src/CMS/__tests__/test-utils.tsVueApp/src/CMS/__tests__/url.test.tsVueApp/src/CMS/__tests__/use-server-table.test.tsVueApp/src/CMS/__tests__/use-url-filtered-table.test.tsVueApp/src/CMS/components/ActivityRow.vueVueApp/src/CMS/components/ContentBlock.vueVueApp/src/CMS/components/ContentDiffDialog.vueVueApp/src/CMS/components/DateRangeFilter.vueVueApp/src/CMS/components/DeleteRestoreButtons.vueVueApp/src/CMS/components/EditButton.vueVueApp/src/CMS/components/FileFormDialog.vueVueApp/src/CMS/components/InlineFileUpload.vueVueApp/src/CMS/components/LeftNavMenuDialog.vueVueApp/src/CMS/components/LeftNavMenuSettingsFields.vueVueApp/src/CMS/components/Link.vueVueApp/src/CMS/components/LinkCollections.vueVueApp/src/CMS/components/ListCard.vueVueApp/src/CMS/components/ListCardField.vueVueApp/src/CMS/components/ModifiedStamp.vueVueApp/src/CMS/components/PermissionChips.vueVueApp/src/CMS/components/PermissionSelector.vueVueApp/src/CMS/components/PersonSelector.vueVueApp/src/CMS/components/RecentActivity.vueVueApp/src/CMS/components/StatusIcon.vueVueApp/src/CMS/composables/use-content-diff-viewer.tsVueApp/src/CMS/composables/use-server-table.tsVueApp/src/CMS/composables/use-url-filtered-table.tsVueApp/src/CMS/file-types.tsVueApp/src/CMS/pages/BulkEncrypt.vueVueApp/src/CMS/pages/CmsHome.vueVueApp/src/CMS/pages/ContentBlockEdit.vueVueApp/src/CMS/pages/ContentBlockHistory.vueVueApp/src/CMS/pages/ContentBlocks.vueVueApp/src/CMS/pages/FileAuditLog.vueVueApp/src/CMS/pages/Files.vueVueApp/src/CMS/pages/ImportFiles.vueVueApp/src/CMS/pages/LeftNavEdit.vueVueApp/src/CMS/pages/LeftNavMenus.vueVueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CMS/router/index.tsVueApp/src/CMS/router/routes.tsVueApp/src/CMS/services/cms-options-service.tsVueApp/src/CMS/types/index.tsVueApp/src/CTS/constants.tsVueApp/src/CTS/pages/ManageEpas.vueVueApp/src/CTS/pages/ManageMilestones.vueVueApp/src/CTS/router/index.tsVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/EffortLeftNav.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/router/index.tsVueApp/src/Students/router/index.tsVueApp/src/components/BreadcrumbHeading.vueVueApp/src/components/RichTextEditor.vueVueApp/src/components/SortableList.vueVueApp/src/components/__tests__/rich-text-editor.test.tsVueApp/src/components/__tests__/sortable-list.test.tsVueApp/src/composables/RequireLogin.tsVueApp/src/composables/ViperFetch.tsVueApp/src/composables/__tests__/use-audit-colors.test.tsVueApp/src/composables/__tests__/use-reorder.test.tsVueApp/src/composables/use-audit-colors.tsVueApp/src/composables/use-reorder.tsVueApp/src/composables/use-route-focus.tsVueApp/src/config/colors.tsVueApp/src/layouts/LeftNav.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/shared/__tests__/create-spa-router.test.tsVueApp/src/shared/__tests__/strip-trailing-slashes.test.tsVueApp/src/shared/bootstrap-spa.tsVueApp/src/shared/create-spa-router.tsVueApp/src/shared/createSpaRouter.tsVueApp/src/shared/strip-trailing-slashes.tsVueApp/src/styles/base.cssVueApp/src/styles/colors.css
💤 Files with no reviewable changes (1)
- VueApp/src/shared/createSpaRouter.ts
Comment on lines
+21
to
+32
| scrollBehavior: (to, from, savedPosition) => { | ||
| // Browser back/forward restores the position the user left this entry at; | ||
| // honour it before any of the path/hash defaults below. | ||
| if (savedPosition) { | ||
| return savedPosition | ||
| } | ||
| // Scroll to the fragment target on router-driven hash navigation, | ||
| // but not on query-only changes where the hash merely persists. | ||
| if (to.hash && (to.path !== from.path || to.hash !== from.hash)) { | ||
| return { el: to.hash } | ||
| } | ||
| return to.path === from.path ? false : { left: 0, top: 0 } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)create-spa-router\.ts$|VueApp/src/shared/.*' || true
echo "== file excerpt =="
if [ -f VueApp/src/shared/create-spa-router.ts ]; then
nl -ba VueApp/src/shared/create-spa-router.ts | sed -n '1,80p'
fi
echo "== router tests/search =="
rg -n "scrollBehavior|savedPosition|create-spa-router|Router|samePathHash|hash removal|fragment" VueApp/src -S || true
echo "== package/vue-router context =="
rg -n "vue-router|`@/shared/create-spa-router`" VueApp/src package.json VueApp -S || true
echo "== behavioral probe: current function invariants =="
python3 - <<'PY'
from pathlib import Path
p = Path('VueApp/src/shared/create-spa-router.ts')
if not p.exists():
print('missing file')
raise SystemExit
text = p.read_text()
start = text.index('scrollBehavior:')
end = text.index('}', start) + 1
print(text[start:end])
PYRepository: ucdavis/VIPER
Length of output: 459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
awk '{printf "%6d\t%s\n", NR, $0}' VueApp/src/shared/create-spa-router.ts | sed -n '1,80p'
echo "== router tests/search =="
rg -n "scrollBehavior|savedPosition|create-spa-router|Router|samePathHash|hash removal|fragment|to\\.hash|from\\.hash" VueApp/src -S || true
echo "== package/vue-router context =="
rg -n "vue-router|`@/shared/create-spa-router`" package.json VueApp/src -S || true
echo "== behavioral probe: current function invariant =="
python3 - <<'PY'
from pathlib import Path
p = Path('VueApp/src/shared/create-spa-router.ts')
text = p.read_text()
print(text)
PYRepository: ucdavis/VIPER
Length of output: 35799
🌐 Web query:
Vue Router scrollBehavior same-path hash removal to.hash empty from hash returns false behavior
💡 Result:
In Vue Router, when navigating to the same route and removing a hash (resulting in to.hash being empty), the scrollBehavior function is triggered as part of the navigation process [1]. If you return false (or any falsy value like null or undefined) from scrollBehavior, Vue Router will perform no scrolling action [2][3]. Here is a breakdown of how this behavior works and how to handle it: 1. Triggering Scroll Behavior: Modern versions of Vue Router ensure that scrollBehavior is triggered even when navigating to the same route if the hash changes [1]. When you navigate from a route with a hash to one without (or where the hash is removed), to.hash will be empty [4][5]. 2. Returning False: Returning false or any falsy value explicitly tells the router not to perform any scrolling [2][3]. This is the expected and documented behavior if you wish to prevent the default scroll-to-top behavior that often occurs on route navigation [2][3]. 3. Custom Logic: To selectively disable scrolling (e.g., when removing a hash but staying on the same route), you can add logic inside scrollBehavior to compare the to and from objects. For example: javascript scrollBehavior(to, from, savedPosition) { // If navigating to the same route and the hash is being removed if (to.name === from.name && from.hash && !to.hash) { return false; // Prevent scrolling } // Otherwise, define your default scroll behavior if (to.hash) { return { el: to.hash, behavior: 'smooth' }; } return { top: 0 }; } If you find that the browser still scrolls despite returning false, it may be due to the browser's native behavior following a link or an interaction that Vue Router cannot fully suppress, in which case preventing the default event on the initiating link or using a delay might be necessary [6][7].
Citations:
- 1: vuejs/vue-router@57d8042
- 2: https://v3.router.vuejs.org/guide/advanced/scroll-behavior
- 3: https://v3.router.vuejs.org/guide/advanced/scroll-behavior.html
- 4: https://router.vuejs.org/guide/advanced/scroll-behavior
- 5: https://github.com/vuejs/router/blob/main/packages/router/src/scrollBehavior.ts
- 6: Navigation to the same route fails (anchor, hash) vuejs/vue-router#1668
- 7: Scroll behavior not respecting offset on hashchange vuejs/router#1147
Reset scroll when removing a hash.
Navigating from /page#section to /page leaves to.hash empty, so the hash branch is skipped and Line 32 returns false; Vue Router then performs no scroll action and keeps the previous fragment position.
- if (to.hash && (to.path !== from.path || to.hash !== from.hash)) {
- return { el: to.hash }
+ if (to.hash !== from.hash) {
+ return to.hash ? { el: to.hash } : { left: 0, top: 0 }
}Add a regression test for fragment removal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scrollBehavior: (to, from, savedPosition) => { | |
| // Browser back/forward restores the position the user left this entry at; | |
| // honour it before any of the path/hash defaults below. | |
| if (savedPosition) { | |
| return savedPosition | |
| } | |
| // Scroll to the fragment target on router-driven hash navigation, | |
| // but not on query-only changes where the hash merely persists. | |
| if (to.hash && (to.path !== from.path || to.hash !== from.hash)) { | |
| return { el: to.hash } | |
| } | |
| return to.path === from.path ? false : { left: 0, top: 0 } | |
| scrollBehavior: (to, from, savedPosition) => { | |
| // Browser back/forward restores the position the user left this entry at; | |
| // honour it before any of the path/hash defaults below. | |
| if (savedPosition) { | |
| return savedPosition | |
| } | |
| // Scroll to the fragment target on router-driven hash navigation, | |
| // but not on query-only changes where the hash merely persists. | |
| if (to.hash !== from.hash) { | |
| return to.hash ? { el: to.hash } : { left: 0, top: 0 } | |
| } | |
| return to.path === from.path ? false : { left: 0, top: 0 } |
🤖 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 `@VueApp/src/shared/create-spa-router.ts` around lines 21 - 32, Update the
scrollBehavior callback to reset scroll when navigation removes a hash, so
transitioning from a hashed URL to the same path without a hash returns the
top-left position instead of false. Preserve saved-position restoration and
hash-target scrolling, and add a regression test covering fragment removal.
Slice 3/3 of the restacked CMS migration (tip file states). The Vue 3 management UI: files, content blocks with QEditor and version diffs, left-nav editor, link collections, import and bulk-encrypt tools, the redesigned hub with the recent-activity rail, shared sortable-list / server-table / dialog components, mobile card mode, and the full frontend test suite. Completes the stack; the tree now matches the development branch tip exactly.
- extract getAuditActionColor to a shared composable so the CMS file audit and the Effort audit list (and any future audit surface) color the same verb classes identically, per PR #253 review feedback - extract ChangeDetailDiff from AuditList's three layouts to remove the old/new diff markup duplication flagged by the JSCPD gate
feature/clinical-scheduler-audit-log merges to main first and reworked the same regions of AuditList (filter card, search row, change-detail markup). Adopt its version wholesale - including AuditChangeDetail, byte-identical, extended to the desktop layout - and re-apply only the shared audit palette, so the later main merge resolves cleanly. ChangeDetailDiff is superseded and removed.
Adds a "view source" toolbar command so editors can inspect and edit the underlying HTML directly (img alt text, a href/target) until richer attribute dialogs exist.
…ady on the page The "Add Left-Nav Menu" link used onMounted to check ?add=1, so re-clicking it while already on the page did nothing (dialog didn't open, query param wasn't stripped, nav highlight was wrong). Switched to the same consume-the-query-flag watcher pattern Files.vue already uses for ?upload=1.
…system selects - File upload's "VIPER app (folder)" select now notes the folder can't be changed after upload, matching the pattern Content Blocks already uses for its section-path select - Content block's "System" select now explains the Viper/Public choice (which site renders the block) instead of relying on the label alone
Deleted/all views gain a sortable "Purges" column (purgeOn = deletedOn + 30 days; sorts by the API's deletedOn for an identical order), a matching card-mode field, and a warning badge when a file's purge is within 7 days or overdue -- previously only visible per-file via the "Deleted ... purges ..." row label.
… page - Strip a leading "viper" path segment on the import page to match the legacy paste format, mirroring the backend's ResolveSource handling
- Add a "Diff vs current" row action alongside the existing diff-with-previous-version action, so reviewers can see what changed since a given saved version without it being superseded first - Disable the action for rows whose block has since been deleted
…PA base path - Both create-spa-router and bootstrap-spa stripped only a single trailing slash from VITE_VIPER_HOME, so a misconfigured value with more than one (e.g. "/2///") would leave a stray slash in the router base / area-path prefix check
- Mirrors the backend's new blank-URL rejection with the same client-side check the empty-text validation already uses, so the error surfaces immediately instead of after a round trip
- Add a shared RichTextEditor wrapping QEditor with accessible toolbar button names, an action-describing view-source toggle, and a named editing region; adopt it in the CMS block editor and the CTS EPA/milestone editors - Size editor headings and the format-size preview to the published .content-block scale so editing is WYSIWYG - Return to the block listing after save, showing the block title in the toast - Merge VIPER section and Page into one Section/Page column, drop Modified under 1440px, and rework the card (two-up fields, inline header actions, tighter spacing) so the listing fits iPad - Reveal hidden permissions from the "+N more" chip and tag recently updated rows in the Modified column - Document RichTextEditor in DESIGN.md
- share the list-card grid/compact styles via base.css (Content Blocks + Files) - Files: 2-up card fields with inline row actions, cards below md, drop Modified below lg
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@VueApp/src/CMS/components/InlineFileUpload.vue`:
- Around line 242-246: Update overwriteInPlace to append
item.conflict.existingModifiedOn to the FormData before calling putForm,
preserving the stale-edit guard used by FileFormDialog. Extend the inline
overwrite test to assert that lastModifiedOn is included with the expected
existing modification value.
In `@VueApp/src/CMS/components/RecentActivity.vue`:
- Around line 16-21: Replace the failed-state div in RecentActivity with the
StatusBanner component, set its type to error, and preserve the existing
“Couldn't load recent activity.” message and styling intent so the failure is
announced as an alert.
- Around line 175-219: Extract the shared CMS files API interactions used by
loadFiles and loadDeletedFiles into a dedicated cms-files-service.ts, following
the existing cms-options-service.ts pattern. Expose service methods for active
and deleted file queries, then update both loaders in RecentActivity.vue to use
that service instead of calling get with cms/files/ directly.
- Around line 239-252: Update loadActivity so failed.value also reflects partial
source failures, not only the all-rejected case; retain the existing
empty-source behavior and successfully loaded items. Add the minimal UI
treatment near the activity list to indicate when any source in results failed,
using the component’s existing error-state presentation where available.
- Around line 95-127: Add a request sequence or captured viewer identity to
openLatestDiff and the shared history diff flows in ContentBlockHistory.vue.
After each asynchronous operation, verify the request still owns the active
viewer before calling openViewer, applyDiff, closeViewer, or failViewer; return
immediately for stale requests so newer dialog state cannot be changed.
In `@VueApp/src/CMS/pages/ContentBlocks.vue`:
- Around line 341-353: Extract the shared Promise-wrapped confirmation behavior
from deleteBlock, Files.vue’s confirmAction, and BulkEncrypt.encryptSelected
into src/composables/use-confirm-dialog.ts. Expose a composable helper that
preserves the existing dialog options and resolves true only on confirmation,
then update all three consumers to call it and remove their duplicated inline
implementations.
In `@VueApp/src/components/__tests__/sortable-list.test.ts`:
- Around line 127-139: Update the test around the VueDraggable "end" emission to
simulate SortableJS updating the v-model first: set the mounted list’s
modelValue to the reordered items with Alpha at index 2, then emit "end" with
oldIndex 0 and newIndex 2. Change the reorder assertion to expect Alpha at index
2 while preserving the existing event-count and status-message checks.
In `@VueApp/src/components/BreadcrumbHeading.vue`:
- Around line 12-13: Remove the self-evident markup/prop comments in
VueApp/src/components/BreadcrumbHeading.vue lines 12-13 and
VueApp/src/CMS/components/PermissionChips.vue lines 72-73; alternatively, retain
only a concise rationale in BreadcrumbHeading.vue explaining that heading-link
styling is centralized in base.css. No other implementation changes are needed.
In `@VueApp/src/components/SortableList.vue`:
- Line 327: Update the `@media` breakpoint in `SortableList.vue` from the
`599px` unit to its equivalent `rem` value, preserving the existing responsive
threshold and media-query behavior.
- Around line 207-220: Replace the inline style mutations in the FLIP animation
block of SortableList with the Web Animations API, using element.animate to
apply the transform and transition timing while preserving the existing delta
movement, easing, duration, and cleanup behavior. Remove the
requestAnimationFrame and transitionend listener used solely for element.style
updates.
In `@VueApp/src/Effort/pages/AuditList.vue`:
- Around line 190-193: Update ClinicalScheduler to stop importing its local
getAuditActionColor helper from ../utils/audit-actions and reuse the shared
helper from use-audit-colors.ts instead. Ensure its audit badges use the same
verb-prefix color mapping as AuditList, removing only the duplicate import or
implementation needed for that switch.
In `@VueApp/src/styles/base.css`:
- Around line 585-636: Add published heading styles for the CMS-supported h4 and
h5 elements in the global .content-block and .content-block-editor
.q-editor__content selectors, matching the existing .q-menu h4 and h5 sizes.
Remove the unused .q-menu h6 rule unless the CMS exposes H6, in which case add
corresponding published H6 styles as well.
🪄 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 Plus
Run ID: c0e41e34-ddf9-4fc3-99c8-9327efb49e7e
📒 Files selected for processing (100)
DESIGN.mdVueApp/src/CAHFS/router/index.tsVueApp/src/CMS/__tests__/cms-home.test.tsVueApp/src/CMS/__tests__/content-block-edit-diff.test.tsVueApp/src/CMS/__tests__/content-block-edit-save.test.tsVueApp/src/CMS/__tests__/content-block-history.test.tsVueApp/src/CMS/__tests__/content-blocks.test.tsVueApp/src/CMS/__tests__/content-diff-dialog.test.tsVueApp/src/CMS/__tests__/date-range-filter.test.tsVueApp/src/CMS/__tests__/file-form-dialog-conflict.test.tsVueApp/src/CMS/__tests__/file-form-dialog.test.tsVueApp/src/CMS/__tests__/files.test.tsVueApp/src/CMS/__tests__/import-files.test.tsVueApp/src/CMS/__tests__/inline-file-upload.test.tsVueApp/src/CMS/__tests__/left-nav-edit.test.tsVueApp/src/CMS/__tests__/left-nav-menu-dialog.test.tsVueApp/src/CMS/__tests__/left-nav-menus.test.tsVueApp/src/CMS/__tests__/link-collections.test.tsVueApp/src/CMS/__tests__/link.test.tsVueApp/src/CMS/__tests__/modified-stamp.test.tsVueApp/src/CMS/__tests__/permission-selector.test.tsVueApp/src/CMS/__tests__/person-selector.test.tsVueApp/src/CMS/__tests__/recent-activity.test.tsVueApp/src/CMS/__tests__/router-canonicalization.test.tsVueApp/src/CMS/__tests__/test-utils.tsVueApp/src/CMS/__tests__/url.test.tsVueApp/src/CMS/__tests__/use-server-table.test.tsVueApp/src/CMS/__tests__/use-url-filtered-table.test.tsVueApp/src/CMS/components/ActivityRow.vueVueApp/src/CMS/components/ContentBlock.vueVueApp/src/CMS/components/ContentDiffDialog.vueVueApp/src/CMS/components/DateRangeFilter.vueVueApp/src/CMS/components/DeleteRestoreButtons.vueVueApp/src/CMS/components/EditButton.vueVueApp/src/CMS/components/FileFormDialog.vueVueApp/src/CMS/components/InlineFileUpload.vueVueApp/src/CMS/components/LeftNavMenuDialog.vueVueApp/src/CMS/components/LeftNavMenuSettingsFields.vueVueApp/src/CMS/components/Link.vueVueApp/src/CMS/components/LinkCollections.vueVueApp/src/CMS/components/ListCard.vueVueApp/src/CMS/components/ListCardField.vueVueApp/src/CMS/components/ModifiedStamp.vueVueApp/src/CMS/components/PermissionChips.vueVueApp/src/CMS/components/PermissionSelector.vueVueApp/src/CMS/components/PersonSelector.vueVueApp/src/CMS/components/RecentActivity.vueVueApp/src/CMS/components/StatusIcon.vueVueApp/src/CMS/composables/use-content-diff-viewer.tsVueApp/src/CMS/composables/use-server-table.tsVueApp/src/CMS/composables/use-url-filtered-table.tsVueApp/src/CMS/file-types.tsVueApp/src/CMS/pages/BulkEncrypt.vueVueApp/src/CMS/pages/CmsHome.vueVueApp/src/CMS/pages/ContentBlockEdit.vueVueApp/src/CMS/pages/ContentBlockHistory.vueVueApp/src/CMS/pages/ContentBlocks.vueVueApp/src/CMS/pages/FileAuditLog.vueVueApp/src/CMS/pages/Files.vueVueApp/src/CMS/pages/ImportFiles.vueVueApp/src/CMS/pages/LeftNavEdit.vueVueApp/src/CMS/pages/LeftNavMenus.vueVueApp/src/CMS/pages/ManageLinkCollections.vueVueApp/src/CMS/router/index.tsVueApp/src/CMS/router/routes.tsVueApp/src/CMS/services/cms-options-service.tsVueApp/src/CMS/types/index.tsVueApp/src/CTS/constants.tsVueApp/src/CTS/pages/ManageEpas.vueVueApp/src/CTS/pages/ManageMilestones.vueVueApp/src/CTS/router/index.tsVueApp/src/ClinicalScheduler/router/index.tsVueApp/src/Computing/router/index.tsVueApp/src/Effort/components/EffortLeftNav.vueVueApp/src/Effort/pages/AuditList.vueVueApp/src/Effort/router/index.tsVueApp/src/Students/router/index.tsVueApp/src/components/BreadcrumbHeading.vueVueApp/src/components/RichTextEditor.vueVueApp/src/components/SortableList.vueVueApp/src/components/__tests__/rich-text-editor.test.tsVueApp/src/components/__tests__/sortable-list.test.tsVueApp/src/composables/RequireLogin.tsVueApp/src/composables/ViperFetch.tsVueApp/src/composables/__tests__/use-audit-colors.test.tsVueApp/src/composables/__tests__/use-reorder.test.tsVueApp/src/composables/use-audit-colors.tsVueApp/src/composables/use-reorder.tsVueApp/src/composables/use-route-focus.tsVueApp/src/config/colors.tsVueApp/src/layouts/LeftNav.vueVueApp/src/layouts/ViperLayout.vueVueApp/src/shared/__tests__/create-spa-router.test.tsVueApp/src/shared/__tests__/strip-trailing-slashes.test.tsVueApp/src/shared/bootstrap-spa.tsVueApp/src/shared/create-spa-router.tsVueApp/src/shared/createSpaRouter.tsVueApp/src/shared/strip-trailing-slashes.tsVueApp/src/styles/base.cssVueApp/src/styles/colors.css
💤 Files with no reviewable changes (1)
- VueApp/src/shared/createSpaRouter.ts
Comment on lines
+242
to
+246
| async function overwriteInPlace(item: StagedFile, data: FormData): Promise<CommitResult> { | ||
| const res = await putForm(apiURL + item.conflict!.existingFileGuid, data) | ||
| if (!res.success) throw new Error(res.errors?.[0] ?? `Failed to overwrite ${item.file.name}`) | ||
| return { file: res.result as CmsFile, created: false } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the PUT/editFile handler for cms/files/{guid} and check lastModifiedOn usage.
rg -nP -C4 'lastModifiedOn' --type=cs
rg -nP -C3 'existingModifiedOn' VueApp/src/CMSRepository: ucdavis/VIPER
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files matching relevant names =="
fd -a 'InlineFileUpload\.vue|FileFormDialog|CmsFileNameCheck|file-form-dialog-conflict\.test|inline-file-upload\.test|CMS' . | sed 's#^\./##' | head -200
echo
echo "== git status/stat =="
git diff --stat || true
git ls-files | rg 'VueApp/src/CMS|CmsFileNameCheck|file-form-dialog-conflict|inline-file-upload|CmsFile|FileForm|CMS' | head -200Repository: ucdavis/VIPER
Length of output: 8676
🏁 Script executed:
#!/bin/bash
set -u
echo "== InlineFileUpload relevant sections =="
nl -ba VueApp/src/CMS/components/InlineFileUpload.vue | sed -n '210,265p'
echo
nl -ba VueApp/src/CMS/components/InlineFileUpload.vue | sed -n '1,80p'
echo
echo "== FileFormDialog overwrite section =="
nl -ba VueApp/src/CMS/components/FileFormDialog.vue | sed -n '1,260p' | rg -n -C6 'lastModifiedOn|existingModifiedOn|overwrite|PUT|formData|formData\.append|toFormData|buildUpload|form|conflict' || true
echo
echo "== tests relevant sections =="
nl -ba VueApp/src/CMS/__tests__/file-form-dialog-conflict.test.ts | sed -n '120,180p'
echo
nl -ba VueApp/src/CMS/__tests__/inline-file-upload.test.ts | sed -n '220,290p'
echo
echo "== controller relevant sections =="
nl -ba web/Areas/CMS/Controllers/CMSFilesController.cs | sed -n '1,260p'
echo
echo "== all lastModifiedOn repo occurrences (cs files) =="
rg -n -C4 'lastModifiedOn' --glob '*.cs' .Repository: ucdavis/VIPER
Length of output: 7895
🏁 Script executed:
#!/bin/bash
set -u
echo "== use awk/Python to print relevant file sections with line numbers =="
python3 - <<'PY'
import pathlib, re
files = [
("vueapp/src/CMS/components/InlineFileUpload.vue", 210, 265, "inline_upload_210_265"),
("vueapp/src/CMS/components/InlineFileUpload.vue", 1, 80, "inline_upload_1_80"),
("vueapp/src/CMS/components/FileFormDialog.vue", 1, 280, "file_dialog_1_280"),
("vueapp/src/CMS/__tests__/file-form-dialog-conflict.test.ts", 100, 190, "conflict_test_100_190"),
("vueapp/src/CMS/__tests__/inline-file-upload.test.ts", 210, 300, "inline_test_210_300"),
("web/Areas/CMS/Controllers/CMSFilesController.cs", 1, 280, "files_controller_1_280"),
]
for f, lo, hi, label in files:
p = pathlib.Path(f)
print(f"\n===== {label}: {f} =====")
print(p)
text = p.read_text(errors="replace")
lines = text.splitlines()
for i in range(lo, min(hi, len(lines)) + 1):
line = lines[i-1] if i <= len(lines) else ""
marker = "*" if re.search(r'(lastModifiedOn|overwrite|Put|PUT|conflict|existingModifiedOn|FormData|toFormData|upload)', line, re.I) else ""
print(f"{i:5}: {marker:5} {line}")
PY
echo
echo "== CMSFileService relevant upload sections =="
python3 - <<'PY'
import pathlib, re
p = pathlib.Path("web/Areas/CMS/Services/CmsFileService.cs")
text = p.read_text(errors="replace")
lines = text.splitlines()
for start, end, label in [(400, 500, "CmsFileService.AssertNotStale and upload handlers")]:
print(f"\n===== {label}: lines {start}-{end} =====")
for i in range(start, min(end, len(lines)) + 1):
line = lines[i-1]
marker = "*" if re.search(r'(lastModifiedOn|AssertNotStale|EditFile|Upload|Put|PUT|FileToPeople|ModifiedOn|ModifiedBy|Guid)', line, re.I) else ""
print(f"{i:5}: {marker:5} {line}")
PYRepository: ucdavis/VIPER
Length of output: 7457
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate exact file under repo root =="
git ls-files | rg 'InlineFileUpload\.vue|FileFormDialog\.vue|cms-file-service' | head -50
echo
python3 - <<'PY'
import pathlib, re
files = [
("VueApp/src/CMS/components/InlineFileUpload.vue", 1, 85, "InlineFileUpload imports/types"),
("VueApp/src/CMS/components/InlineFileUpload.vue", 220, 270, "InlineFileUpload overwriteInPlace/buildUploadData"),
("VueApp/src/CMS/components/FileFormDialog.vue", 1, 285, "FileFormDialog conflict/overwrite"),
("VueApp/src/CMS/__tests__/file-form-dialog-conflict.test.ts", 100, 190, "FileFormDialog conflict test"),
("VueApp/src/CMS/__tests__/inline-file-upload.test.ts", 210, 300, "InlineFileUpload tests"),
("web/Areas/CMS/Controllers/CMSFilesController.cs", 80, 190, "CMSFilesController Put edit route"),
("web/Areas/CMS/Services/CmsFileService.cs", 200, 450, "CmsFileService upload/edit implementation"),
("web/Areas/CMS/Models/DTOs/CmsFileRequests.cs", 1, 220, "CmsFileUpdateRequest DTO"),
]
for path, lo, hi, label in files:
p = pathlib.Path(path)
print(f"\n===== {label}: {path} lines {lo}-{hi} =====")
text = p.read_text(errors="replace")
lines = text.splitlines()
for i in range(lo, min(hi, len(lines)) + 1):
line = lines[i-1]
marker = "*" if re.search(r'(lastModifiedOn|overwrite|Put|PUT|formData|FormData|toFormData|upload|EditFile|Update|Request|ModifiedOn|ModifiedBy|AssertNotStale|conflict|existingModifiedOn)', line, re.I) else ""
print(f"{i:5}: {marker:5} {line}")
PYRepository: ucdavis/VIPER
Length of output: 50369
Send the stale-edit guard when overwriting in place.
overwriteInPlace() calls buildUploadData(), which only sends file, allowPublicAccess, and permissions, so the PUT to cms/files/{existingFileGuid} lacks the required lastModifiedOn guard used by the FileFormDialog overwrite path. Append item.conflict.existingModifiedOn to the overwrite FormData and add an assertion in the inline overwrite test.
🤖 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 `@VueApp/src/CMS/components/InlineFileUpload.vue` around lines 242 - 246,
Update overwriteInPlace to append item.conflict.existingModifiedOn to the
FormData before calling putForm, preserving the stale-edit guard used by
FileFormDialog. Extend the inline overwrite test to assert that lastModifiedOn
is included with the expected existing modification value.
Source: Coding guidelines
Comment on lines
+16
to
+21
| <div | ||
| v-else-if="failed" | ||
| class="text-body2 text-grey-8 q-mt-xs" | ||
| > | ||
| Couldn't load recent activity. | ||
| </div> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"Couldn't load recent activity" should announce to assistive tech.
This is a genuine fetch failure (unlike ContentDiffDialog's deliberately-muted "identical versions" note), yet it's a plain div with no role. As per coding guidelines, {**/*.vue,**/*.cshtml}: "Use StatusBanner component for Vue SPAs. For type="error", it auto-maps to role="alert"". Route this through StatusBanner with type="error" so screen-reader users are notified when the widget fails to load.
🤖 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 `@VueApp/src/CMS/components/RecentActivity.vue` around lines 16 - 21, Replace
the failed-state div in RecentActivity with the StatusBanner component, set its
type to error, and preserve the existing “Couldn't load recent activity.”
message and styling intent so the failure is announced as an alert.
Source: Coding guidelines
Comment on lines
+95
to
+127
| async function openLatestDiff(b: CmsContentBlock, label: string) { | ||
| openViewer(label) | ||
| const listParams = createUrlSearchParams({ contentBlockId: b.contentBlockId, page: 1, perPage: 1 }) | ||
| const [blockRes, listRes] = await Promise.all([ | ||
| get(apiURL + "CMS/content/" + b.contentBlockId), | ||
| get(apiURL + "CMS/content/history?" + listParams), | ||
| ]) | ||
| const previous = ((listRes.result ?? []) as CmsContentHistoryAudit[])[0] | ||
| if (listRes.success && !previous) { | ||
| closeViewer() | ||
| $q.notify({ type: "info", message: "No edit history for this block yet." }) | ||
| return | ||
| } | ||
| if (!listRes.success || !blockRes.success) { | ||
| failViewer("Failed to load the latest change") | ||
| return | ||
| } | ||
| const res = await post( | ||
| apiURL + "CMS/content/" + b.contentBlockId + "/history/" + previous.contentHistoryId + "/diff", | ||
| { | ||
| content: (blockRes.result as CmsContentBlock).content, | ||
| }, | ||
| ) | ||
| if (res.success) { | ||
| const diff = res.result as CmsContentHistoryDiff | ||
| applyDiff( | ||
| diff, | ||
| `Changes from ${diffStamp(diff.oldModifiedOn, diff.oldModifiedBy)} to ${diffStamp(b.modifiedOn, b.modifiedBy)}`, | ||
| ) | ||
| } else { | ||
| failViewer(res.errors?.[0] ?? "Failed to load the latest change") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -g '*use-content-diff-viewer*'
cat -n VueApp/src/CMS/composables/use-content-diff-viewer.tsRepository: ucdavis/VIPER
Length of output: 3017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate RecentActivity and content diff usages"
fd -g '*RecentActivity.vue' .
rg -n "useContentDiffViewer|openLatestDiff|loadContentBlock|requestSeq|openViewer\\(|closeViewer\\(|applyDiff\\(" VueApp/src || true
echo
echo "## RecentActivity outline"
ast-grep outline VueApp/src/CMS/components/RecentActivity.vue || true
echo
echo "## RecentActivity relevant lines"
sed -n '1,190p' VueApp/src/CMS/components/RecentActivity.vue | cat -n
echo
echo "## ContentBlock relevant line ranges if mentioned"
fd -g '*ContentBlock.vue' .
while IFS= read -r f; do
echo "### $f"
rg -n "openViewer|closeViewer|applyDiff|loadContentBlock|requestedName|requestSeq" "$f" || true
done < <(fd -g '*ContentBlock.vue' .)Repository: ucdavis/VIPER
Length of output: 10921
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "## ContentBlock.vue relevant lines"
sed -n '1,80p' VueApp/src/CMS/components/ContentBlock.vue | cat -n
echo
echo "## ContentBlockEdit.vue diff-flow relevant lines"
sed -n '460,525p' VueApp/src/CMS/pages/ContentBlockEdit.vue | cat -n
echo
echo "## ContentBlockHistory.vue diff-flow relevant lines"
sed -n '250,315p' VueApp/src/CMS/pages/ContentBlockHistory.vue | cat -n
echo
echo "## use-server-table.ts guard"
sed -n '45,75p' VueApp/src/CMS/composables/use-server-table.ts | cat -n
echo
echo "## diff request sequencing summary"
python3 - <<'PY'
from pathlib import Path
for p in [
Path("VueApp/src/CMS/components/RecentActivity.vue"),
Path("VueApp/src/CMS/components/ContentBlock.vue"),
Path("VueApp/src/CMS/pages/ContentBlockEdit.vue"),
Path("VueApp/src/CMS/pages/ContentBlockHistory.vue"),
Path("VueApp/src/CMS/composables/use-content-diff-viewer.ts"),
]:
text = p.read_text()
print(p, {
"openLatestDiff": "openLatestDiff" in text,
"loadContentBlock": "loadContentBlock" in text,
"ContentBlockHistoryDiffDialogOpen": "ContentBlockHistoryDiffDialogOpen" in text,
"viewerSeq/requestSeq/capturedName": any(s in text for s in ["viewerSeq", "requestSeq", "capturedName", "selectedRowId"]),
"noSeqNoRequestGuard": "seq !== requestSeq" not in text and "requestId !== diffRequestSeq" not in text,
})
PYRepository: ucdavis/VIPER
Length of output: 10811
Guard out-of-order diff requests in openLatestDiff.
useContentDiffViewer does not de-dupe or cancel in-flight views — openViewer(...), applyDiff(...), closeViewer(), and failViewer(...) can still run after a newer viewer has replaced the title/state. Add a sequence guard/captured name after each async step so stale responses cannot close a newer dialog or apply a stale diff. Apply the same guard to the shared history diff flows in ContentBlockHistory.vue too.
🤖 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 `@VueApp/src/CMS/components/RecentActivity.vue` around lines 95 - 127, Add a
request sequence or captured viewer identity to openLatestDiff and the shared
history diff flows in ContentBlockHistory.vue. After each asynchronous
operation, verify the request still owns the active viewer before calling
openViewer, applyDiff, closeViewer, or failViewer; return immediately for stale
requests so newer dialog state cannot be changed.
Comment on lines
+175
to
+219
| async function loadFiles(): Promise<ActivityItem[]> { | ||
| const params = createUrlSearchParams({ | ||
| status: "active", | ||
| page: 1, | ||
| perPage: PER_SOURCE, | ||
| sortBy: "modifiedOn", | ||
| descending: "true", | ||
| }) | ||
| const res = await get(apiURL + "cms/files/?" + params) | ||
| if (!res.success) throw new Error("files") | ||
| return ((res.result ?? []) as CmsFile[]).map((f) => ({ | ||
| key: "file-" + f.fileGuid, | ||
| icon: "description", | ||
| typeLabel: "File", | ||
| label: f.friendlyName, | ||
| to: { name: "CmsFiles", query: { search: f.friendlyName } }, | ||
| modifiedOn: f.modifiedOn, | ||
| modifiedBy: f.modifiedBy, | ||
| actions: [fileAuditAction(f.fileGuid)], | ||
| })) | ||
| } | ||
|
|
||
| async function loadDeletedFiles(): Promise<ActivityItem[]> { | ||
| const params = createUrlSearchParams({ | ||
| status: "deleted", | ||
| page: 1, | ||
| perPage: PER_SOURCE, | ||
| sortBy: "deletedOn", | ||
| descending: "true", | ||
| }) | ||
| const res = await get(apiURL + "cms/files/?" + params) | ||
| if (!res.success) throw new Error("deletedFiles") | ||
| return ((res.result ?? []) as CmsFile[]).map((f) => ({ | ||
| key: "trash-" + f.fileGuid, | ||
| icon: "delete", | ||
| typeLabel: "File", | ||
| verb: "deleted", | ||
| label: f.friendlyName, | ||
| to: { name: "CmsFiles", query: { status: "deleted", search: f.friendlyName } }, | ||
| // Soft delete stamps ModifiedOn/ModifiedBy alongside DeletedOn, so the deleter is correct. | ||
| modifiedOn: f.deletedOn ?? f.modifiedOn, | ||
| modifiedBy: f.modifiedBy, | ||
| actions: [fileAuditAction(f.fileGuid)], | ||
| })) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
loadFiles/loadDeletedFiles call cms/files/ directly instead of through a dedicated service.
Based on learnings, when a CMS Vue component calls cms/files/ endpoints directly, the recommendation is a dedicated, consistent "CMS files" service abstraction centralizing these interactions, not one-off inline calls per component. loadFiles and loadDeletedFiles both hit cms/files/ directly here, alongside other components already doing the same. Consider extracting a shared cms-files-service.ts (parallel to cms-options-service.ts) once this pattern appears in a third consumer.
🤖 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 `@VueApp/src/CMS/components/RecentActivity.vue` around lines 175 - 219, Extract
the shared CMS files API interactions used by loadFiles and loadDeletedFiles
into a dedicated cms-files-service.ts, following the existing
cms-options-service.ts pattern. Expose service methods for active and deleted
file queries, then update both loaders in RecentActivity.vue to use that service
instead of calling get with cms/files/ directly.
Source: Learnings
Comment on lines
+239
to
+252
| async function loadActivity() { | ||
| const sources = [ | ||
| ...(showBlocks ? [loadBlocks()] : []), | ||
| ...(showFiles ? [loadFiles(), loadDeletedFiles()] : []), | ||
| ...(showLeftNavs ? [loadLeftNavs()] : []), | ||
| ] | ||
| const results = await Promise.allSettled(sources) | ||
| const loaded = results.filter((r) => r.status === "fulfilled").flatMap((r) => r.value) | ||
| failed.value = sources.length > 0 && results.every((r) => r.status === "rejected") | ||
| items.value = loaded | ||
| .sort((a, b) => new Date(b.modifiedOn).getTime() - new Date(a.modifiedOn).getTime()) | ||
| .slice(0, MAX_ITEMS) | ||
| loading.value = false | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Partial source failures are silently swallowed.
failed.value is only set when every source rejects (results.every((r) => r.status === "rejected")). If, say, loadBlocks() rejects but loadFiles()/loadDeletedFiles() succeed, the widget renders an incomplete list with zero indication that blocks failed to load — a quiet partial-error propagation gap.
🔧 Minimal fix to surface partial failures
const results = await Promise.allSettled(sources)
const loaded = results.filter((r) => r.status === "fulfilled").flatMap((r) => r.value)
- failed.value = sources.length > 0 && results.every((r) => r.status === "rejected")
+ const anyRejected = results.some((r) => r.status === "rejected")
+ failed.value = sources.length > 0 && results.every((r) => r.status === "rejected")
+ partiallyFailed.value = anyRejected && !failed.value
items.value = loaded(exact UI treatment for the partial state is up to you — a subtle inline note near the list would suffice).
🤖 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 `@VueApp/src/CMS/components/RecentActivity.vue` around lines 239 - 252, Update
loadActivity so failed.value also reflects partial source failures, not only the
all-rejected case; retain the existing empty-source behavior and successfully
loaded items. Add the minimal UI treatment near the activity list to indicate
when any source in results failed, using the component’s existing error-state
presentation where available.
Comment on lines
+12
to
+13
| // Breadcrumb-style page heading for subpages: the parent crumb links back, the | ||
| // current page is plain text. Link styling comes from base.css (h1 a rules). |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Keep comments to non-obvious rationale. These comments describe straightforward markup or prop behavior rather than explaining a necessary design decision.
VueApp/src/components/BreadcrumbHeading.vue#L12-L13: replace with a concise rationale about centralizing heading-link styling inbase.css, or remove it.VueApp/src/CMS/components/PermissionChips.vue#L72-L73: remove the self-evidentstackedprop description.
As per coding guidelines, comments must explain why, not what, and be used sparingly.
📍 Affects 2 files
VueApp/src/components/BreadcrumbHeading.vue#L12-L13(this comment)VueApp/src/CMS/components/PermissionChips.vue#L72-L73
🤖 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 `@VueApp/src/components/BreadcrumbHeading.vue` around lines 12 - 13, Remove the
self-evident markup/prop comments in VueApp/src/components/BreadcrumbHeading.vue
lines 12-13 and VueApp/src/CMS/components/PermissionChips.vue lines 72-73;
alternatively, retain only a concise rationale in BreadcrumbHeading.vue
explaining that heading-link styling is centralized in base.css. No other
implementation changes are needed.
Source: Coding guidelines
Comment on lines
+207
to
+220
| el.style.transition = "none" | ||
| el.style.transform = `translateY(${delta}px)` | ||
| requestAnimationFrame(() => { | ||
| el.style.transition = `transform ${SLIDE_MS}ms cubic-bezier(0.22, 1, 0.36, 1)` | ||
| el.style.transform = "" | ||
| }) | ||
| el.addEventListener( | ||
| "transitionend", | ||
| () => { | ||
| el.style.transition = "" | ||
| el.style.transform = "" | ||
| }, | ||
| { once: true }, | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid runtime inline styles for the FLIP animation.
Replace the element.style mutations with a Web Animations API animation so this component complies with the no-inline-styles rule.
Proposed fix
- el.style.transition = "none"
- el.style.transform = `translateY(${delta}px)`
- requestAnimationFrame(() => {
- el.style.transition = `transform ${SLIDE_MS}ms cubic-bezier(0.22, 1, 0.36, 1)`
- el.style.transform = ""
- })
- el.addEventListener(
- "transitionend",
- () => {
- el.style.transition = ""
- el.style.transform = ""
- },
- { once: true },
- )
+ el.animate(
+ [
+ { transform: `translateY(${delta}px)` },
+ { transform: "translateY(0)" },
+ ],
+ {
+ duration: SLIDE_MS,
+ easing: "cubic-bezier(0.22, 1, 0.36, 1)",
+ },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| el.style.transition = "none" | |
| el.style.transform = `translateY(${delta}px)` | |
| requestAnimationFrame(() => { | |
| el.style.transition = `transform ${SLIDE_MS}ms cubic-bezier(0.22, 1, 0.36, 1)` | |
| el.style.transform = "" | |
| }) | |
| el.addEventListener( | |
| "transitionend", | |
| () => { | |
| el.style.transition = "" | |
| el.style.transform = "" | |
| }, | |
| { once: true }, | |
| ) | |
| el.animate( | |
| [ | |
| { transform: `translateY(${delta}px)` }, | |
| { transform: "translateY(0)" }, | |
| ], | |
| { | |
| duration: SLIDE_MS, | |
| easing: "cubic-bezier(0.22, 1, 0.36, 1)", | |
| }, | |
| ) |
🤖 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 `@VueApp/src/components/SortableList.vue` around lines 207 - 220, Replace the
inline style mutations in the FLIP animation block of SortableList with the Web
Animations API, using element.animate to apply the transform and transition
timing while preserving the existing delta movement, easing, duration, and
cleanup behavior. Remove the requestAnimationFrame and transitionend listener
used solely for element.style updates.
Source: Coding guidelines
|
|
||
| /* On phones the row becomes a stacked card: handle + controls share a top bar, | ||
| the body drops to its own full-width line below. */ | ||
| @media (width <= 599px) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a rem breakpoint.
Replace 599px with its rem equivalent.
As per coding guidelines: “Use rem (not px) for spacing, sizing, and typography.”
🤖 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 `@VueApp/src/components/SortableList.vue` at line 327, Update the `@media`
breakpoint in `SortableList.vue` from the `599px` unit to its equivalent `rem`
value, preserving the existing responsive threshold and media-query behavior.
Source: Coding guidelines
Comment on lines
+190
to
193
| <StatusBadge :color="getAuditActionColor(props.row.action)"> | ||
| {{ props.row.action }} | ||
| </StatusBadge> | ||
| </q-td> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the two getAuditActionColor implementations for divergence/duplication.
fd -e ts -i audit-colors -x cat -n {}
fd -e ts -i audit-actions -x cat -n {}Repository: ucdavis/VIPER
Length of output: 3785
🏁 Script executed:
#!/bin/bash
# Inspect the imports and audit badge usages across the candidates to determine whether
# ClinicalScheduler is duplicating the shared palette or already using the new composable.
sed -n '1,80p' VueApp/src/ClinicalScheduler/utils/audit-actions.ts
printf '\n--- audit-actions usages ---\n'
rg -n "getAuditActionColor|audit-actions|AccessFileDenied|Created from rotation|Removed from rotation" VueApp/src/ClinicalScheduler VueApp/src/Effort -g '*.ts' -g '*.vue' || true
printf '\n--- use-audit-colors usages ---\n'
rg -n "getAuditActionColor|use-audit-colors" VueApp/src -g '*.ts' -g '*.vue' || trueRepository: ucdavis/VIPER
Length of output: 5506
Share the ClinicalScheduler audit badge color helper too.
ClinicalScheduler still imports its own getAuditActionColor() from ../utils/audit-actions, which duplicates the Action-color mapping instead of sharing the verb-prefix palette in use-audit-colors.ts. That leaves badge colors divergent across audit views.
🤖 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 `@VueApp/src/Effort/pages/AuditList.vue` around lines 190 - 193, Update
ClinicalScheduler to stop importing its local getAuditActionColor helper from
../utils/audit-actions and reuse the shared helper from use-audit-colors.ts
instead. Ensure its audit badges use the same verb-prefix color mapping as
AuditList, removing only the duplicate import or implementation needed for that
switch.
Source: Coding guidelines
Comment on lines
+585
to
+636
| /* Headings inside sanitized CMS content (v-html). Shared by the live content | ||
| block (CMS/components/ContentBlock.vue) and the history diff view | ||
| (CMS/components/ContentDiffDialog.vue) so both render block headings the same. | ||
| The content-block editor (CMS/pages/ContentBlockEdit.vue) opts its QEditor | ||
| content area in via .content-block-editor so the editing surface previews the | ||
| same heading sizes it will publish at, rather than the compressed | ||
| .q-page-container chrome sizes it would otherwise inherit. Declared globally | ||
| because scoped styles reach neither v-html children nor QEditor's content. */ | ||
| .content-block h1, | ||
| .content-block-editor .q-editor__content h1 { | ||
| font-size: 2rem; | ||
| line-height: 1.2; | ||
| } | ||
|
|
||
| .content-block h2, | ||
| .content-block-editor .q-editor__content h2 { | ||
| font-size: 1.5rem; | ||
| line-height: 1.25; | ||
| } | ||
|
|
||
| .content-block h3, | ||
| .content-block-editor .q-editor__content h3 { | ||
| font-size: 1.25rem; | ||
| line-height: 1.3; | ||
| } | ||
|
|
||
| /* QEditor's heading picker (the format_size dropdown) previews each option by rendering a real | ||
| heading inside its dropdown menu. That menu teleports to <body>, outside the .content-block-editor | ||
| scope above, so without this the previews fall back to browser-default sizes (h2 ~60px) and wildly | ||
| overstate the output. Pin them to the same published sizes so the picker matches what the editor | ||
| and live block render. QEditor is the only place in the app that renders headings inside a q-menu. */ | ||
| .q-menu h1 { | ||
| font-size: 2rem; | ||
| } | ||
|
|
||
| .q-menu h2 { | ||
| font-size: 1.5rem; | ||
| } | ||
|
|
||
| .q-menu h3 { | ||
| font-size: 1.25rem; | ||
| } | ||
|
|
||
| .q-menu h4 { | ||
| font-size: 1.1rem; | ||
| } | ||
|
|
||
| .q-menu h5, | ||
| .q-menu h6 { | ||
| font-size: 1rem; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether .content-block h4/h5/h6 exist elsewhere, and whether any CMS/CTS
# rich-text toolbar exposes headings beyond h3.
rg -n '\.content-block h[4-6]|content-block-editor.*h[4-6]' VueApp/src/styles/base.css
rg -n 'format_size|toolbar\s*=|CMS_.*TOOLBAR|headings' VueApp/src/CMS/pages/ContentBlockEdit.vueRepository: ucdavis/VIPER
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ContentBlockEdit relevant lines =="
sed -n '40,70p' VueApp/src/CMS/pages/ContentBlockEdit.vue
sed -n '410,445p' VueApp/src/CMS/pages/ContentBlockEdit.vue
echo
echo "== Search format_size usage elsewhere =="
rg -n 'format_size|toolbar\s*=' VueApp/src || true
echo
echo "== CSS heading styles context =="
sed -n '560,640p' VueApp/src/styles/base.css
echo
echo "== CMS/CTS/toolbars text search relevant terms =="
rg -n 'CMS|TOOLBAR|toolbar|format_size|heading|Headings?|H[1-6]' VueApp/src | head -200 || trueRepository: ucdavis/VIPER
Length of output: 28207
Add matching public styles for CMS heading options H4 and H5.
editorToolbar exposes h4 and h5, but the published .content-block styles only pin h1-h3. The .q-menu preview values (h4: 1.1rem, h5/h6: 1rem) will not match published output. Add matching .content-block-editor .q-editor__content h4/h5 and remove the unused .q-menu h6 if CMS doesn’t offer H6, or add published H6 styles too.
🤖 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 `@VueApp/src/styles/base.css` around lines 585 - 636, Add published heading
styles for the CMS-supported h4 and h5 elements in the global .content-block and
.content-block-editor .q-editor__content selectors, matching the existing
.q-menu h4 and h5 sizes. Remove the unused .q-menu h6 rule unless the CMS
exposes H6, in which case add corresponding published H6 styles as well.
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.
Slice 3 of 4 (stacks on #252; the diff shows only this slice). Built from the CI-verified branch tip — see #251 for why the earlier 6-PR stack (#245-#250) was replaced. Merging this completes the CMS migration as originally scoped; slice 4 (#255) adds delegated block editing on top.
Scope — the CMS management SPA (Vue 3 + Quasar)
/CMS/ManageFiles: server-paged list with shareable URL-synced filters, upload/edit dialog with name-conflict and 409 edit-conflict handling, audit trail, import and bulk-encrypt tools.SortableList+ live-region announcements) and independent settings/items save; link collections management.use-server-tablewith stale-response guard,use-url-filtered-table,use-reorder, route-focus a11y), mobile card mode for the five dense list tables, WCAG-focused polish, and the full frontend test suite (1,026 tests).Deploy notes (whole stack)
CMS:LegacyWebrootPath(import tool),viperfiles.txtkey file must exist, optionalCMS:FileStorageRoot/CMS:ProfilePhotoPath/CMS:EncryptionKeyFileoverrides,CMS:DownloadRateLimitlimits,Cms:TrashPurgeEnabledstays false until the legacy VIPER 1 purge is retired.Development→ TEST first, per repo convention.