refactor: migrate frontend to Pinia + performance cleanup#566
refactor: migrate frontend to Pinia + performance cleanup#566NagariaHussain wants to merge 13 commits intofrappe:developfrom
Conversation
…llet) Install Pinia, create useUserStore with isWikiManager computed, and migrate SpaceList.vue as the first consumer to prove the architecture end-to-end. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e 2) Add canAccessWiki, shouldUseChangeRequestMode, data, roles, isLoading, fetch, reload, reset to the Pinia user store. Migrate MainLayout, Contributions, ContributionReview, and router to use the store instead of direct userResource/composable imports. Rewire useChangeRequest.js to delegate to the store (keeping thin wrappers for Phase 4 consumers). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate session state to Pinia store with cross-store communication (session → user). Delete src/data/ directory — no remaining consumers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Atomic migration of the entire currentChangeRequest consumer tree into a single Pinia setup store, replacing both useChangeRequestMode and useChangeRequest composables. All 6 consuming components now use useChangeRequestStore; the old composable is deleted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All cleanup was already done in Phases 3-4. Verified: no stale imports, build passes clean, browser-tested end-to-end. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add ensureChangeRequest helper to CR store, replacing 6 repeated init-if-missing patterns in WikiDocumentList - Collapse two overlapping watchers in SpaceDetails into one, removing duplicate loadChanges + crTree.reload on every mount - Deduplicate 4 loadCrPage triggers in WikiDocumentPanel into a single combined watch on [currentChangeRequest.name, doc_key] - Skip user fetch in router guard when data is already loaded Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… (Phase 2) - Extract useChangeTypeDisplay composable (shared by ContributionBanner + ContributionReview) - Consolidate banner switch-case computeds into BANNER_CONFIG lookup map - Hide raw resources from CR store; expose isSubmitting/isArchiving/isMerging/etc getters - ContributionBanner reads store directly, drops 8 props (keeps only mergeDisabled) - Extract useTreeDialogs composable from WikiDocumentList (15+ refs, 10+ functions) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace JSON.parse(JSON.stringify(tree)) with a changeTypeMap computed that components look up by doc_key, and cache the in-flight loadChanges promise to prevent duplicate API calls from overlapping watchers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace blank loading states with shimmer skeletons for a smoother perceived load when opening large spaces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request migrates the frontend from a composable-based state management pattern to centralized Pinia store management. It adds the Pinia dependency and creates three new stores: useChangeRequestStore, useUserStore, and useSessionStore. Two composables are introduced: useChangeTypeDisplay for change type rendering logic and useTreeDialogs for tree dialog operations. Components throughout the codebase are refactored to obtain state from stores instead of direct composables and prop drilling. Legacy session and user data files are removed and replaced by their store equivalents. Router initialization and main application setup are updated to register and use Pinia. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
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)
frontend/src/pages/ContributionReview.vue (1)
430-449:⚠️ Potential issue | 🟡 MinorAfter merge,
changeRequest.reload()is called butchanges.submit(...)could fail silently.If
changes.submit(...)on Line 438 throws, the error is caught by the outer catch, but the catch block then attempts to fetch conflicts or show an error toast — which would be misleading since the merge itself succeeded. Consider separating the post-merge refresh from the merge try/catch, or at minimum handling the post-merge refresh errors distinctly.🛡️ Suggested approach
async function handleApprove() { try { await mergeResource.submit({ name: props.changeRequestId }); toast.success(__('Change request merged')); if (crStore.currentChangeRequest?.name === props.changeRequestId) { crStore.currentChangeRequest = null; } changeRequest.reload(); - await changes.submit({ name: props.changeRequestId, scope: 'summary' }); + changes.submit({ name: props.changeRequestId, scope: 'summary' }).catch(() => {}); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ContributionReview.vue` around lines 430 - 449, The merge try/catch in handleApprove currently mixes the merge operation (mergeResource.submit) with a post-merge refresh (changeRequest.reload + changes.submit) so a failure in changes.submit is treated like a merge failure; split the responsibilities by awaiting mergeResource.submit and handling its errors in the existing catch (keeping toast.success only when merge succeeds), then move changeRequest.reload and changes.submit into a separate try/catch block executed only after merge succeeds; if the post-merge changes.submit fails, do not call fetchConflicts or treat it as a merge ValidationError—log or toast a distinct "refresh failed" message and ensure crStore.currentChangeRequest clearing remains consistent (refer to handleApprove, mergeResource.submit, changeRequest.reload, changes.submit, crStore.currentChangeRequest, fetchConflicts, hasConflicts).
🧹 Nitpick comments (19)
frontend/src/composables/useTreeDialogs.js (3)
5-6:spaceIdandexpandedNodesare implicitly expected to beRef<T>— consider a JSDoc note.All action functions access
spaceId.value(e.g., line 59) andexpandedNodes.value(lines 79, 186). A caller passing a plain string or plain object would silently getundefined, breaking everyensureChangeRequestcall without an obvious error. A brief JSDoc comment (or atoRef/toValuecall at the top) would make the contract explicit.♻️ Suggested clarification
+/** + * `@param` {import('vue').Ref<string>} spaceId - reactive space ID ref + * `@param` {import('vue').Ref<Record<string, boolean>>} expandedNodes - reactive map of expanded node keys + * `@param` {Function} emit - component emit function + */ export function useTreeDialogs(spaceId, expandedNodes, emit) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useTreeDialogs.js` around lines 5 - 6, The functions in useTreeDialogs expect spaceId and expandedNodes to be Vue Refs but this is only implicit; update useTreeDialogs to either document the contract with a JSDoc note above the function (clarifying that spaceId and expandedNodes must be Ref<T>) or convert the inputs to refs at the top (e.g., via toRef/toValue) so callers can pass plain values safely, and ensure all internal uses (e.g., where ensureChangeRequest, spaceId.value, expandedNodes.value are referenced) work consistently with the normalized refs.
156-196: No URL format validation for external links.
createExternalLinkandupdateExternalLinkonly guard against empty strings (lines 162–165 and 211–214) but accept arbitrary values like"not a url". Since these URLs become clickable navigation targets for wiki readers, a basic URL validity check would prevent obvious mis-entries.♻️ Suggested guard (add before the try block in both functions)
+ try { + new URL(externalLinkUrl.value.trim()); + } catch { + toast.warning(__('Please enter a valid URL')); + return; + } + try { if (!(await crStore.ensureChangeRequest(spaceId.value))) {Apply the same guard in
updateExternalLinkforeditExternalLinkUrl.Also applies to: 205-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useTreeDialogs.js` around lines 156 - 196, Add a basic URL format validation before the try block in both createExternalLink and updateExternalLink: validate externalLinkUrl.value (for createExternalLink) and editExternalLinkUrl.value (for updateExternalLink) with a simple URL-check (e.g., regex or URL constructor) and show toast.warning(__('Invalid URL')) and return if it fails; keep the existing empty-string checks, then proceed with the existing try/await logic so only syntactically valid URLs are accepted before calling crStore.createPage / crStore.updatePage.
119-147:renameDocumentsilently swallows errors — missingconsole.error.Every other async action (
createDocument,deleteDocument,createExternalLink,updateExternalLink) callsconsole.error(...)in its catch block before showing the toast.renameDocument(line 144) only shows the toast, which makes debugging rename failures harder. Minor consistency gap.♻️ Proposed fix
} catch (error) { + console.error('Error renaming document:', error); toast.error(error.messages?.[0] || __('Error updating title')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useTreeDialogs.js` around lines 119 - 147, The catch block in renameDocument is missing a console.error call (unlike createDocument/deleteDocument/createExternalLink/updateExternalLink), so add a console.error(error) at the start of the catch to log the thrown error before calling toast.error; locate the renameDocument function and its catch (related to crStore.updatePage/crStore.ensureChangeRequest and toast.error) and insert console.error(error) to mirror the other async handlers.frontend/src/components/NestedDraggable.vue (2)
56-67: Badge theme for'added'is"blue"— inconsistent withuseChangeTypeDisplaywhich mapsadded → 'green'.As noted in the composable review,
ContributionReview.vueusesgetChangeTheme('added')which returns'green', while this component hardcodestheme="blue"for the same state. This creates an inconsistent visual language for the user.Consider consuming
useChangeTypeDisplayhere, or at minimum aligning the hardcoded values.♻️ Suggested approach using the composable
<script setup> +import { useChangeTypeDisplay } from '@/composables/useChangeTypeDisplay'; ... +const { getChangeTheme, getChangeLabel } = useChangeTypeDisplay();Then in the template, replace the hardcoded badges with a single dynamic one:
-<Badge v-if="changeTypeMap.get(element.doc_key) === 'added'" variant="subtle" theme="blue" size="sm"> - {{ __('New') }} -</Badge> -<Badge v-else-if="changeTypeMap.get(element.doc_key) === 'deleted'" variant="subtle" theme="red" size="sm"> - {{ __('Deleted') }} -</Badge> -<Badge v-else-if="changeTypeMap.get(element.doc_key) === 'modified'" variant="subtle" theme="blue" size="sm"> - {{ __('Modified') }} -</Badge> -<Badge v-else-if="changeTypeMap.get(element.doc_key) === 'reordered'" variant="subtle" theme="orange" size="sm"> - {{ __('Reordered') }} -</Badge> +<Badge v-if="changeTypeMap.has(element.doc_key)" variant="subtle" :theme="getChangeTheme(changeTypeMap.get(element.doc_key))" size="sm"> + {{ getChangeLabel(changeTypeMap.get(element.doc_key)) }} +</Badge>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/NestedDraggable.vue` around lines 56 - 67, NestedDraggable.vue hardcodes badge themes (e.g., theme="blue" for 'added') which conflicts with the central mapping in useChangeTypeDisplay (getChangeTheme returns 'green' for 'added'); fix by importing and using the composable (useChangeTypeDisplay) inside this component and replace the multiple hardcoded <Badge> branches (that check changeTypeMap.get(element.doc_key)) with a single dynamic Badge that uses getChangeTheme(changeType) for the theme (and the composable's label helper if available) so the component uses the same change-type-to-theme mapping as ContributionReview.vue.
146-154: Prop typeMapwon't trigger Vue's prop validation warning on non-Map values.Vue's runtime prop check for
type: Mapworks correctly, but consumers passing a plain object instead of aMapwon't get a helpful error — they'll just seeundefinedfrom.get()calls. Avalidatorfunction could guard against this:validator: (v) => v instanceof Map,This is a minor defensive measure since all current callers are under your control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/NestedDraggable.vue` around lines 146 - 154, The changeTypeMap prop on the component is declared with type: Map but lacks a runtime guard, so consumers passing a plain object will quietly break when calling changeTypeMap.get; update the defineProps declaration for changeTypeMap to add a validator that returns v instanceof Map (keeping the existing default: () => new Map()) so Vue will warn when a non-Map is supplied and prevent undefined .get() usage; locate the changeTypeMap definition in the props block inside the component where defineProps is used and add the validator function there.frontend/src/components/DraftContributionPanel.vue (1)
115-121: Potential race condition:loadCrPagemay run beforeinitChangeRequestresolves ifspaceIdis falsy.When
spaceIdis not provided, the code skipsinitChangeRequest/loadChangesbut still callsloadCrPage()at Line 120.loadCrPageguards oncrStore.currentChangeRequest(Line 96) so it will bail out safely, but this means the component shows "Draft not found" even though the real issue is a missingspaceId. Consider adding a more explicit guard or user-facing message for this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DraftContributionPanel.vue` around lines 115 - 121, The onMounted block may call loadCrPage when props.spaceId is falsy, causing a misleading "Draft not found" message; update the logic in the onMounted handler so you only call loadCrPage after a successful init when props.spaceId is present (i.e., await crStore.initChangeRequest(props.spaceId) && crStore.currentChangeRequest before calling loadCrPage) or alternatively add an explicit UI path when props.spaceId is missing (display a clear "spaceId required" / "No space selected" message) to avoid the misleading draft-not-found state; update references in onMounted, props.spaceId, crStore.initChangeRequest, crStore.loadChanges, loadCrPage and crStore.currentChangeRequest accordingly.frontend/src/stores/session.js (2)
7-14: Cookie parsing may misparse values containing&or=.
URLSearchParamswill split on&and=, so a cookie whose value contains either character (e.g., URL-encoded paths) would be mis-parsed. For theuser_idcookie this is safe in practice, but a more robust approach is to iteratedocument.cookie.split('; ')directly:🛡️ Suggested alternative
function getCookieUser() { - const cookies = new URLSearchParams(document.cookie.split('; ').join('&')); - let user = cookies.get('user_id'); + let user = null; + for (const cookie of document.cookie.split('; ')) { + const [name, ...rest] = cookie.split('='); + if (name === 'user_id') { + user = decodeURIComponent(rest.join('=')); + break; + } + } if (user === 'Guest') { user = null; } return user; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/session.js` around lines 7 - 14, The getCookieUser function currently builds a URLSearchParams from document.cookie which misparses cookie values containing '&' or '='; instead iterate document.cookie.split('; ') and for each pair split on the first '=' to get name and value, trim the name, if name === 'user_id' decodeURIComponent the value, treat 'Guest' as null and return that value; update getCookieUser to use this direct parsing approach (split per-cookie, first-index '=' split, decodeURIComponent) to robustly handle embedded '&' or '=' in cookie values.
37-44: Logout error path is unhandled — user may get stuck.If the
logoutAPI call fails (e.g., network error), there's noonErrorhandler. The user remains in an ambiguous state — they intended to log out but the UI didn't navigate or give feedback. Consider adding anonErrorhandler (at minimum a toast), or optimistically clearing local state regardless of server response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/session.js` around lines 37 - 44, The logout flow lacks error handling: update the createResource call for logout to handle failures by adding an onError handler (or perform optimistic local cleanup) so the user isn’t left stuck; specifically, in the logout resource that calls createResource, add an onError callback that calls useUserStore().reset(), sets user.value = getCookieUser() (or clears cookies/session), shows a toast/error notification, and navigates with router.replace({ name: 'Login' }) as appropriate so both logout and failure paths consistently clear local state and provide user feedback.frontend/src/stores/user.js (1)
17-19:isLoadingstaysfalseduring re-fetches sinceuserResource.dataretains stale data.
computed(() => !userResource.data)is onlytruebefore the first successful fetch. Duringreload(), the olddatapersists, soisLoadingremainsfalse. If consumers depend on knowing when a reload is in progress, consider also exposinguserResource.loadingor combining both signals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/user.js` around lines 17 - 19, isLoading currently checks only userResource.data so it never becomes true during reloads; change isLoading to reflect both presence of data and the resource's loading state by using userResource.loading (or combine userResource.loading || !userResource.data) so reload() transitions are observable; update the computed named isLoading in frontend/src/stores/user.js to reference userResource.loading (or a combined expression) and ensure any consumers still using roles or data keep their current computed dependencies (data, roles).plans/pinia-refactor.md (1)
1-3: Consider whetherplans/should be committed to the repository.This is an internal migration plan with phase completion timestamps, dev credentials, and ephemeral context. It's valuable for PR review but may add noise to the repo long-term. Consider moving it to the PR description, a project wiki, or an ADR format if you intend to keep it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/pinia-refactor.md` around lines 1 - 3, The committed plans/pinia-refactor.md contains internal, ephemeral details (phase timestamps and dev credentials) and should not remain in the repo as-is; either remove the file from the commit and add it to the PR description or a secured project wiki/ADR, or redact sensitive data and convert it to a sanitized ADR-style doc if you intend to keep it. Update the branch by deleting or moving plans/pinia-refactor.md (or stripping credentials/timestamps and renaming to an ADR), and if this content must be excluded from future commits add the plans/ directory or the specific filename to .gitignore or remove it from history if already merged. Ensure the header "Pinia State Management Migration Plan (Tracer Bullet Approach)" is preserved only in the sanitized/moved version for traceability.frontend/src/composables/useChangeTypeDisplay.js (1)
41-88: This composable holds no reactive state — it's effectively a set of pure utility functions.Since there's no reactive state or lifecycle, the composable wrapper doesn't add value over plain named exports. That said, it follows Vue convention and leaves room for future reactive additions, so this is a style preference only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/composables/useChangeTypeDisplay.js` around lines 41 - 88, The file wraps pure utility functions in a Vue composable (useChangeTypeDisplay) even though there's no reactive state; refactor by removing the wrapper and exporting the functions as plain named exports (e.g., export function getConfig(changeType), export function getChangeIcon, getChangeIconClass, getChangeTheme, getChangeLabel, export function getChangeDescription) so callers import the utilities directly; ensure the original helper names (getConfig, getChangeIcon, getChangeIconClass, getChangeTheme, getChangeLabel, getChangeDescription) remain unchanged and update any imports/usages elsewhere from "useChangeTypeDisplay()" to direct named imports.frontend/src/pages/ContributionReview.vue (1)
353-356:isManagerwraps an already-computed store value in anothercomputed.
userStore.isWikiManageris already acomputedref in the store. The localisManagerre-wrap is harmless but redundant — you can useuserStore.isWikiManagerdirectly in the template/other computeds. Same applies toisOwnerwhich does need thecomputedsince it combines two sources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ContributionReview.vue` around lines 353 - 356, The local computed isManager redundantly re-wraps the already-computed userStore.isWikiManager; remove the extra computed and use userStore.isWikiManager directly everywhere (keep isOwner as a computed because it compares changeRequest.doc?.owner with userStore.data?.name). Update any template or other computed references from isManager to userStore.isWikiManager and delete the isManager declaration to avoid unnecessary wrapping.frontend/src/pages/SpaceDetails.vue (3)
434-445: DuplicateloadChangescall —refreshTreealready calls it.
handleArchiveChangeRequestcallscrStore.loadChanges()on Line 440 and thenrefreshTree()on Line 441, which internally callscrStore.loadChanges()again (Line 415). The promise-caching in the store mitigates the duplicate network request only if the first hasn't resolved yet, but if it has, a second request fires.The same pattern appears in
handleMergeChangeRequest(Lines 464–465).🧹 Proposed fix for both handlers
async function handleArchiveChangeRequest() { try { await crStore.archiveChangeRequest(); toast.success(__('Change request archived')); crStore.currentChangeRequest = null; await crStore.initChangeRequest(props.spaceId); - await crStore.loadChanges(); await refreshTree(); } catch (error) { toast.error(error.messages?.[0] || __('Error archiving change request')); } }async function handleMergeChangeRequest() { const docKey = currentDraftKey.value; try { await crStore.mergeChangeRequest(); toast.success(__('Change request merged')); crStore.currentChangeRequest = null; await crStore.initChangeRequest(props.spaceId); - await crStore.loadChanges(); await refreshTree();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SpaceDetails.vue` around lines 434 - 445, handleArchiveChangeRequest and handleMergeChangeRequest each call crStore.loadChanges() and then call refreshTree(), but refreshTree() already calls crStore.loadChanges(), causing a possible duplicate network request; remove the explicit crStore.loadChanges() call from both handlers so they only call crStore.initChangeRequest(...) (or other necessary store updates) and then await refreshTree(), leaving crStore.loadChanges() to be invoked once inside refreshTree(); reference the handlers handleArchiveChangeRequest and handleMergeChangeRequest and the store method crStore.loadChanges() and refreshTree() when making the change.
386-408: Watcher mutates state it also watches — works but is subtle.The watcher tracks
crStore.currentChangeRequest?.namewhile also nulling and re-assigningcurrentChangeRequestinside its body (Lines 394, 398). This creates cascading re-triggers (typically 2–3 invocations per doc change). It behaves correctly today becauseinitChangeRequestdeduplicates viainitChangeRequestPromise, but this coupling is fragile — a future change to the dedup guard could introduce duplicate inits.Consider splitting into two separate watchers: one for
space.docchanges (to reset + init) and one forcrStore.currentChangeRequest?.namechanges (to load changes + tree). This would make the data flow linear and avoid self-triggering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SpaceDetails.vue` around lines 386 - 408, The watcher is mutating state it watches (crStore.currentChangeRequest) causing self-triggered re-runs; split into two watchers: one that watches space.doc and when doc changes sets crStore.currentChangeRequest = null and then calls await crStore.initChangeRequest(props.spaceId) (use immediate as before), and a separate watcher that watches crStore.isChangeRequestMode and crStore.currentChangeRequest?.name and, when the name changes, calls await crStore.loadChanges() and await crTree.reload() but does NOT mutate crStore.currentChangeRequest; keep existing dedup logic in crStore.initChangeRequest and preserve immediate behavior where appropriate.
247-247:isManageris computed but never used.
isManageris declared on Line 247 but is not referenced anywhere in the template or script. It was likely needed before the migration when it was passed as a prop toContributionBanner, which now readsuserStore.isWikiManagerdirectly.🧹 Proposed fix
-const isManager = computed(() => userStore.isWikiManager);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SpaceDetails.vue` at line 247, The computed const isManager in SpaceDetails.vue is unused; remove the declaration "const isManager = computed(() => userStore.isWikiManager);" to eliminate the dead variable (or if it was intended to be passed into ContributionBanner, reintroduce that prop usage instead); locate the const by name and delete it, and ensure any components (e.g., ContributionBanner) continue to read userStore.isWikiManager directly or accept the isManager prop consistently.frontend/src/stores/changeRequest.js (2)
86-104:isLoadingChangeRequestis managed in two places — the action and the resource callbacks.The flag is set to
trueininitChangeRequest(Line 94) but set tofalseindraftChangeRequestResource.onSuccess/onError(Lines 28, 32). This dual-ownership works today but could drift if either location is modified independently.Consider managing it entirely in the function body (e.g., in a
finallyblock) or entirely in the resource callbacks — not both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/changeRequest.js` around lines 86 - 104, The isLoadingChangeRequest flag is being managed in two places (initChangeRequest and draftChangeRequestResource.onSuccess/onError) which risks state drift; centralize responsibility in initChangeRequest by removing any isLoadingChangeRequest toggling from draftChangeRequestResource.onSuccess and onError and instead set isLoadingChangeRequest.value = true before creating initChangeRequestPromise and set it back to false in a finally block inside initChangeRequest (ensuring you only clear it for the active initChangeRequestPromise to handle concurrent calls), leaving draftChangeRequestResource to only handle data updates and errors without touching isLoadingChangeRequest.
178-234: Page actions acceptchangeRequestNameas a parameter while lifecycle actions use internal state — consider unifying.
createPage,updatePage,deletePage,movePage,reorderChildrenall require the caller to passchangeRequestName, whilesubmitForReview,archiveChangeRequest,mergeChangeRequestread fromcurrentChangeRequest.valueinternally. This inconsistency means callers must know when to pass the name and when not to.Consider either making all actions use internal state (since
currentChangeRequestis always set before these are called) or making all accept an explicit name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/changeRequest.js` around lines 178 - 234, Unify change request sourcing by removing the changeRequestName parameter from createPage, updatePage, deletePage, movePage, and reorderChildren and have those functions read the current change request name from currentChangeRequest.value (the same way submitForReview, archiveChangeRequest, mergeChangeRequest do); update each function signature (createPage, updatePage, deletePage, movePage, reorderChildren) to omit changeRequestName and replace the submitted payloads' name field with currentChangeRequest.value.name (and add a guard/error if currentChangeRequest is null), and adjust any callers to stop passing a name.frontend/src/components/ContributionBanner.vue (1)
222-260:__()calls inBANNER_CONFIGare evaluated once at setup time — not reactive to locale changes.
BANNER_CONFIGandDEFAULT_BANNERare plain objects whosetitle/descriptionstrings are computed once when the component mounts. If the app ever supports runtime locale switching, these won't update. Since this is an admin panel where runtime locale changes are rare, this is unlikely to be an issue today.If you do want to future-proof this, wrap it in a
computed:💡 Optional: make banner config reactive
-const BANNER_CONFIG = { +const BANNER_CONFIG = computed(() => ({ Draft: { ... }, ... -}; +})); -const DEFAULT_BANNER = { +const DEFAULT_BANNER = computed(() => ({ ... -}; +})); -const bannerConfig = computed(() => BANNER_CONFIG[changeRequestStatus.value] || DEFAULT_BANNER); +const bannerConfig = computed(() => BANNER_CONFIG.value[changeRequestStatus.value] || DEFAULT_BANNER.value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ContributionBanner.vue` around lines 222 - 260, BANNER_CONFIG and DEFAULT_BANNER use __('...') at module/setup time so their title/description won't update on runtime locale changes; change them to computed properties so __('...') is evaluated reactively (e.g. replace the plain BANNER_CONFIG and DEFAULT_BANNER objects with computed(() => ({ Draft: { title: __('Change Request Draft'), description: __('...'), ...}, ... })) or wrap title/description values in functions that call __('...') on access), updating any code that reads BANNER_CONFIG or DEFAULT_BANNER to use the computed values (keep references to the same identifiers BANNER_CONFIG and DEFAULT_BANNER so lookup code like in the template or methods continues to work).frontend/src/components/WikiDocumentList.vue (1)
311-331: Non-atomic reorder:movePageandreorderChildrenare sequential calls.If
movePagesucceeds butreorderChildrenfails, the tree is left in a partially reordered state. Thecatchblock does emitrefreshto reload the tree, which recovers the UI. Just noting this is a known trade-off — if atomicity matters, consider a single backend endpoint that handles both operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/WikiDocumentList.vue` around lines 311 - 331, applyReorder currently calls crStore.movePage and then crStore.reorderChildren sequentially which can leave the tree partially updated if the second call fails; to fix, implement an atomic operation on the backend or add a single client method that calls one backend endpoint (e.g., crStore.movePageAndReorder or crStore.applyReorderTransaction) that accepts item.doc_key, payload.newParent, payload.newIndex and siblingKeys and performs both operations server-side, then replace the two separate calls in applyReorder with that single method; if changing the backend is not possible, implement a client-side compensating rollback in applyReorder: wrap movePage and reorderChildren in try/catch, on reorderChildren failure call crStore.movePage to revert to the original parent/index (using information from payload and/or a saved original state) before emitting refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/composables/useChangeTypeDisplay.js`:
- Around line 7-32: The theme for the "added" change type is inconsistent:
CHANGE_TYPE_CONFIG in useChangeTypeDisplay.js defines added.theme = 'green' but
NestedDraggable.vue hardcodes theme="blue"; update NestedDraggable.vue to use
the composable instead of the hardcoded string by importing and calling the
composable (useChangeTypeDisplay / getChangeTheme) or otherwise reference
CHANGE_TYPE_CONFIG so the badge uses getChangeTheme('added') (or the
composable-provided value) ensuring the "added" badge theme matches the central
CHANGE_TYPE_CONFIG.
In `@frontend/src/router.js`:
- Around line 83-90: The guard currently calls userStore.fetch() regardless of
the cached session value, causing an unnecessary auth API call; change the
conditional so the fetch only runs when the session indicates a logged-in user
(use sessionStore.isLoggedIn / the local isLoggedIn variable) and userStore.data
is missing (i.e., only call userStore.fetch() when isLoggedIn is true and
!userStore.data), so you avoid triggering the store's AuthenticationError and
the router.push({ name: 'LoginPage' }) side-effect when no session exists.
In `@frontend/src/stores/changeRequest.js`:
- Around line 113-128: loadChanges currently lets network rejections bubble up
(via changesResource.submit) which causes unhandled promise rejections at
callers; update loadChanges (the async function loadChanges and its use of
loadChangesPromise and changesResource.submit) to catch errors inside the
function, log or record the error, clear loadChangesPromise in finally as
already done, and return a safe fallback (e.g., []) so callers awaiting
loadChanges won't receive a rejected promise; ensure the catch is applied to the
promise chain returned from changesResource.submit so loadChanges always
resolves to an array.
In `@frontend/src/stores/user.js`:
- Around line 10-14: The stores are pushing/replacing invalid route names
('LoginPage' in user.js's onError and 'Login' in session.js) which will silently
fail; fix by either (A) adding a named Login route in the router (name: 'Login')
and update both stores to use that exact name, or (B) remove the explicit
router.push/router.replace calls from user.js:onError and session store and rely
on the existing router.beforeEach guard that redirects to /login via
window.location.href; pick one approach and make the change consistently across
both stores (update onError and the session store’s navigation calls) so they
use the same valid mechanism for unauthenticated redirects.
In `@plans/pinia-refactor.md`:
- Around line 155-162: Remove the plaintext credentials "Administrator" /
"admin" from the verification checklist and replace them with a non-sensitive
placeholder or instructions to retrieve credentials from a secure ephemeral
source; specifically edit the "Automated browser testing" section that currently
lists those credentials and either (a) redact the strings "Administrator" and
"admin" to something like <REDACTED> and add a note pointing to the PR
description or an environment/secret store, or (b) move the entire credentials
note out of committed files into the PR description or a wiki/temporary document
that is not tracked. Ensure the change updates the same "Automated browser
testing" bullet and any references to http://wiki.localhost:8000/login so no
plaintext secrets remain in source control.
- Around line 13-19: The Markdown fenced code block that lists the src/stores/
tree (the block starting with ``` and containing "src/stores/ index.js ...
changeRequest.js") is missing a language specifier; update that opening fence to
include a plain language token (e.g., ```text or ```bash) so the block conforms
to MD040 and static analysis passes.
---
Outside diff comments:
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 430-449: The merge try/catch in handleApprove currently mixes the
merge operation (mergeResource.submit) with a post-merge refresh
(changeRequest.reload + changes.submit) so a failure in changes.submit is
treated like a merge failure; split the responsibilities by awaiting
mergeResource.submit and handling its errors in the existing catch (keeping
toast.success only when merge succeeds), then move changeRequest.reload and
changes.submit into a separate try/catch block executed only after merge
succeeds; if the post-merge changes.submit fails, do not call fetchConflicts or
treat it as a merge ValidationError—log or toast a distinct "refresh failed"
message and ensure crStore.currentChangeRequest clearing remains consistent
(refer to handleApprove, mergeResource.submit, changeRequest.reload,
changes.submit, crStore.currentChangeRequest, fetchConflicts, hasConflicts).
---
Nitpick comments:
In `@frontend/src/components/ContributionBanner.vue`:
- Around line 222-260: BANNER_CONFIG and DEFAULT_BANNER use __('...') at
module/setup time so their title/description won't update on runtime locale
changes; change them to computed properties so __('...') is evaluated reactively
(e.g. replace the plain BANNER_CONFIG and DEFAULT_BANNER objects with
computed(() => ({ Draft: { title: __('Change Request Draft'), description:
__('...'), ...}, ... })) or wrap title/description values in functions that call
__('...') on access), updating any code that reads BANNER_CONFIG or
DEFAULT_BANNER to use the computed values (keep references to the same
identifiers BANNER_CONFIG and DEFAULT_BANNER so lookup code like in the template
or methods continues to work).
In `@frontend/src/components/DraftContributionPanel.vue`:
- Around line 115-121: The onMounted block may call loadCrPage when
props.spaceId is falsy, causing a misleading "Draft not found" message; update
the logic in the onMounted handler so you only call loadCrPage after a
successful init when props.spaceId is present (i.e., await
crStore.initChangeRequest(props.spaceId) && crStore.currentChangeRequest before
calling loadCrPage) or alternatively add an explicit UI path when props.spaceId
is missing (display a clear "spaceId required" / "No space selected" message) to
avoid the misleading draft-not-found state; update references in onMounted,
props.spaceId, crStore.initChangeRequest, crStore.loadChanges, loadCrPage and
crStore.currentChangeRequest accordingly.
In `@frontend/src/components/NestedDraggable.vue`:
- Around line 56-67: NestedDraggable.vue hardcodes badge themes (e.g.,
theme="blue" for 'added') which conflicts with the central mapping in
useChangeTypeDisplay (getChangeTheme returns 'green' for 'added'); fix by
importing and using the composable (useChangeTypeDisplay) inside this component
and replace the multiple hardcoded <Badge> branches (that check
changeTypeMap.get(element.doc_key)) with a single dynamic Badge that uses
getChangeTheme(changeType) for the theme (and the composable's label helper if
available) so the component uses the same change-type-to-theme mapping as
ContributionReview.vue.
- Around line 146-154: The changeTypeMap prop on the component is declared with
type: Map but lacks a runtime guard, so consumers passing a plain object will
quietly break when calling changeTypeMap.get; update the defineProps declaration
for changeTypeMap to add a validator that returns v instanceof Map (keeping the
existing default: () => new Map()) so Vue will warn when a non-Map is supplied
and prevent undefined .get() usage; locate the changeTypeMap definition in the
props block inside the component where defineProps is used and add the validator
function there.
In `@frontend/src/components/WikiDocumentList.vue`:
- Around line 311-331: applyReorder currently calls crStore.movePage and then
crStore.reorderChildren sequentially which can leave the tree partially updated
if the second call fails; to fix, implement an atomic operation on the backend
or add a single client method that calls one backend endpoint (e.g.,
crStore.movePageAndReorder or crStore.applyReorderTransaction) that accepts
item.doc_key, payload.newParent, payload.newIndex and siblingKeys and performs
both operations server-side, then replace the two separate calls in applyReorder
with that single method; if changing the backend is not possible, implement a
client-side compensating rollback in applyReorder: wrap movePage and
reorderChildren in try/catch, on reorderChildren failure call crStore.movePage
to revert to the original parent/index (using information from payload and/or a
saved original state) before emitting refresh.
In `@frontend/src/composables/useChangeTypeDisplay.js`:
- Around line 41-88: The file wraps pure utility functions in a Vue composable
(useChangeTypeDisplay) even though there's no reactive state; refactor by
removing the wrapper and exporting the functions as plain named exports (e.g.,
export function getConfig(changeType), export function getChangeIcon,
getChangeIconClass, getChangeTheme, getChangeLabel, export function
getChangeDescription) so callers import the utilities directly; ensure the
original helper names (getConfig, getChangeIcon, getChangeIconClass,
getChangeTheme, getChangeLabel, getChangeDescription) remain unchanged and
update any imports/usages elsewhere from "useChangeTypeDisplay()" to direct
named imports.
In `@frontend/src/composables/useTreeDialogs.js`:
- Around line 5-6: The functions in useTreeDialogs expect spaceId and
expandedNodes to be Vue Refs but this is only implicit; update useTreeDialogs to
either document the contract with a JSDoc note above the function (clarifying
that spaceId and expandedNodes must be Ref<T>) or convert the inputs to refs at
the top (e.g., via toRef/toValue) so callers can pass plain values safely, and
ensure all internal uses (e.g., where ensureChangeRequest, spaceId.value,
expandedNodes.value are referenced) work consistently with the normalized refs.
- Around line 156-196: Add a basic URL format validation before the try block in
both createExternalLink and updateExternalLink: validate externalLinkUrl.value
(for createExternalLink) and editExternalLinkUrl.value (for updateExternalLink)
with a simple URL-check (e.g., regex or URL constructor) and show
toast.warning(__('Invalid URL')) and return if it fails; keep the existing
empty-string checks, then proceed with the existing try/await logic so only
syntactically valid URLs are accepted before calling crStore.createPage /
crStore.updatePage.
- Around line 119-147: The catch block in renameDocument is missing a
console.error call (unlike
createDocument/deleteDocument/createExternalLink/updateExternalLink), so add a
console.error(error) at the start of the catch to log the thrown error before
calling toast.error; locate the renameDocument function and its catch (related
to crStore.updatePage/crStore.ensureChangeRequest and toast.error) and insert
console.error(error) to mirror the other async handlers.
In `@frontend/src/pages/ContributionReview.vue`:
- Around line 353-356: The local computed isManager redundantly re-wraps the
already-computed userStore.isWikiManager; remove the extra computed and use
userStore.isWikiManager directly everywhere (keep isOwner as a computed because
it compares changeRequest.doc?.owner with userStore.data?.name). Update any
template or other computed references from isManager to userStore.isWikiManager
and delete the isManager declaration to avoid unnecessary wrapping.
In `@frontend/src/pages/SpaceDetails.vue`:
- Around line 434-445: handleArchiveChangeRequest and handleMergeChangeRequest
each call crStore.loadChanges() and then call refreshTree(), but refreshTree()
already calls crStore.loadChanges(), causing a possible duplicate network
request; remove the explicit crStore.loadChanges() call from both handlers so
they only call crStore.initChangeRequest(...) (or other necessary store updates)
and then await refreshTree(), leaving crStore.loadChanges() to be invoked once
inside refreshTree(); reference the handlers handleArchiveChangeRequest and
handleMergeChangeRequest and the store method crStore.loadChanges() and
refreshTree() when making the change.
- Around line 386-408: The watcher is mutating state it watches
(crStore.currentChangeRequest) causing self-triggered re-runs; split into two
watchers: one that watches space.doc and when doc changes sets
crStore.currentChangeRequest = null and then calls await
crStore.initChangeRequest(props.spaceId) (use immediate as before), and a
separate watcher that watches crStore.isChangeRequestMode and
crStore.currentChangeRequest?.name and, when the name changes, calls await
crStore.loadChanges() and await crTree.reload() but does NOT mutate
crStore.currentChangeRequest; keep existing dedup logic in
crStore.initChangeRequest and preserve immediate behavior where appropriate.
- Line 247: The computed const isManager in SpaceDetails.vue is unused; remove
the declaration "const isManager = computed(() => userStore.isWikiManager);" to
eliminate the dead variable (or if it was intended to be passed into
ContributionBanner, reintroduce that prop usage instead); locate the const by
name and delete it, and ensure any components (e.g., ContributionBanner)
continue to read userStore.isWikiManager directly or accept the isManager prop
consistently.
In `@frontend/src/stores/changeRequest.js`:
- Around line 86-104: The isLoadingChangeRequest flag is being managed in two
places (initChangeRequest and draftChangeRequestResource.onSuccess/onError)
which risks state drift; centralize responsibility in initChangeRequest by
removing any isLoadingChangeRequest toggling from
draftChangeRequestResource.onSuccess and onError and instead set
isLoadingChangeRequest.value = true before creating initChangeRequestPromise and
set it back to false in a finally block inside initChangeRequest (ensuring you
only clear it for the active initChangeRequestPromise to handle concurrent
calls), leaving draftChangeRequestResource to only handle data updates and
errors without touching isLoadingChangeRequest.
- Around line 178-234: Unify change request sourcing by removing the
changeRequestName parameter from createPage, updatePage, deletePage, movePage,
and reorderChildren and have those functions read the current change request
name from currentChangeRequest.value (the same way submitForReview,
archiveChangeRequest, mergeChangeRequest do); update each function signature
(createPage, updatePage, deletePage, movePage, reorderChildren) to omit
changeRequestName and replace the submitted payloads' name field with
currentChangeRequest.value.name (and add a guard/error if currentChangeRequest
is null), and adjust any callers to stop passing a name.
In `@frontend/src/stores/session.js`:
- Around line 7-14: The getCookieUser function currently builds a
URLSearchParams from document.cookie which misparses cookie values containing
'&' or '='; instead iterate document.cookie.split('; ') and for each pair split
on the first '=' to get name and value, trim the name, if name === 'user_id'
decodeURIComponent the value, treat 'Guest' as null and return that value;
update getCookieUser to use this direct parsing approach (split per-cookie,
first-index '=' split, decodeURIComponent) to robustly handle embedded '&' or
'=' in cookie values.
- Around line 37-44: The logout flow lacks error handling: update the
createResource call for logout to handle failures by adding an onError handler
(or perform optimistic local cleanup) so the user isn’t left stuck;
specifically, in the logout resource that calls createResource, add an onError
callback that calls useUserStore().reset(), sets user.value = getCookieUser()
(or clears cookies/session), shows a toast/error notification, and navigates
with router.replace({ name: 'Login' }) as appropriate so both logout and failure
paths consistently clear local state and provide user feedback.
In `@frontend/src/stores/user.js`:
- Around line 17-19: isLoading currently checks only userResource.data so it
never becomes true during reloads; change isLoading to reflect both presence of
data and the resource's loading state by using userResource.loading (or combine
userResource.loading || !userResource.data) so reload() transitions are
observable; update the computed named isLoading in frontend/src/stores/user.js
to reference userResource.loading (or a combined expression) and ensure any
consumers still using roles or data keep their current computed dependencies
(data, roles).
In `@plans/pinia-refactor.md`:
- Around line 1-3: The committed plans/pinia-refactor.md contains internal,
ephemeral details (phase timestamps and dev credentials) and should not remain
in the repo as-is; either remove the file from the commit and add it to the PR
description or a secured project wiki/ADR, or redact sensitive data and convert
it to a sanitized ADR-style doc if you intend to keep it. Update the branch by
deleting or moving plans/pinia-refactor.md (or stripping credentials/timestamps
and renaming to an ADR), and if this content must be excluded from future
commits add the plans/ directory or the specific filename to .gitignore or
remove it from history if already merged. Ensure the header "Pinia State
Management Migration Plan (Tracer Bullet Approach)" is preserved only in the
sanitized/moved version for traceability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
frontend/package.jsonfrontend/src/components/ContributionBanner.vuefrontend/src/components/DraftContributionPanel.vuefrontend/src/components/NestedDraggable.vuefrontend/src/components/SpaceList.vuefrontend/src/components/WikiDocumentList.vuefrontend/src/components/WikiDocumentPanel.vuefrontend/src/composables/useChangeTypeDisplay.jsfrontend/src/composables/useTreeDialogs.jsfrontend/src/data/session.jsfrontend/src/data/user.jsfrontend/src/layouts/MainLayout.vuefrontend/src/main.jsfrontend/src/pages/ContributionReview.vuefrontend/src/pages/Contributions.vuefrontend/src/pages/SpaceDetails.vuefrontend/src/router.jsfrontend/src/stores/changeRequest.jsfrontend/src/stores/index.jsfrontend/src/stores/session.jsfrontend/src/stores/user.jsplans/pinia-refactor.md
💤 Files with no reviewable changes (2)
- frontend/src/data/user.js
- frontend/src/data/session.js
| const CHANGE_TYPE_CONFIG = { | ||
| added: { | ||
| icon: LucidePlus, | ||
| iconClass: 'bg-green-100 text-green-600', | ||
| theme: 'green', | ||
| label: __('New'), | ||
| }, | ||
| modified: { | ||
| icon: LucidePencil, | ||
| iconClass: 'bg-blue-100 text-blue-600', | ||
| theme: 'blue', | ||
| label: __('Modified'), | ||
| }, | ||
| deleted: { | ||
| icon: LucideTrash2, | ||
| iconClass: 'bg-red-100 text-red-600', | ||
| theme: 'red', | ||
| label: __('Deleted'), | ||
| }, | ||
| reordered: { | ||
| icon: LucideArrowUpDown, | ||
| iconClass: 'bg-amber-100 text-amber-600', | ||
| theme: 'orange', | ||
| label: __('Reordered'), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Theme inconsistency: added is green here but blue in NestedDraggable.vue.
This composable defines added → theme: 'green', but NestedDraggable.vue (Line 56) hardcodes theme="blue" for the added badge. Components using getChangeTheme('added') (e.g., ContributionReview.vue) will render green, while the sidebar tree renders blue for the same change type.
Either update NestedDraggable.vue to consume this composable, or align the hardcoded values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/composables/useChangeTypeDisplay.js` around lines 7 - 32, The
theme for the "added" change type is inconsistent: CHANGE_TYPE_CONFIG in
useChangeTypeDisplay.js defines added.theme = 'green' but NestedDraggable.vue
hardcodes theme="blue"; update NestedDraggable.vue to use the composable instead
of the hardcoded string by importing and calling the composable
(useChangeTypeDisplay / getChangeTheme) or otherwise reference
CHANGE_TYPE_CONFIG so the badge uses getChangeTheme('added') (or the
composable-provided value) ensuring the "added" badge theme matches the central
CHANGE_TYPE_CONFIG.
| let isLoggedIn = sessionStore.isLoggedIn; | ||
| try { | ||
| await userResource.fetch(); | ||
| if (!userStore.data) { | ||
| await userStore.fetch(); | ||
| } | ||
| } catch (error) { | ||
| isLoggedIn = false; | ||
| } |
There was a problem hiding this comment.
Unnecessary API call when isLoggedIn is already false.
isLoggedIn is read from the session cookie before the fetch check, so when there is no session cookie (isLoggedIn === false), the guard still unconditionally enters if (!userStore.data) and fires userStore.fetch(). That call will fail with an AuthenticationError, which the store's onError handler tries to handle by calling router.push({ name: 'LoginPage' }) — but no LoginPage route is registered in this file, so that push silently fails. The catch then sets isLoggedIn = false (already false), and the hard-redirect proceeds correctly, but only after the spurious round-trip.
Gate the fetch on the existing isLoggedIn value:
🛠️ Proposed fix
- try {
- if (!userStore.data) {
- await userStore.fetch();
- }
- } catch (error) {
- isLoggedIn = false;
- }
+ try {
+ if (isLoggedIn && !userStore.data) {
+ await userStore.fetch();
+ }
+ } catch (error) {
+ isLoggedIn = false;
+ }📝 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.
| let isLoggedIn = sessionStore.isLoggedIn; | |
| try { | |
| await userResource.fetch(); | |
| if (!userStore.data) { | |
| await userStore.fetch(); | |
| } | |
| } catch (error) { | |
| isLoggedIn = false; | |
| } | |
| let isLoggedIn = sessionStore.isLoggedIn; | |
| try { | |
| if (isLoggedIn && !userStore.data) { | |
| await userStore.fetch(); | |
| } | |
| } catch (error) { | |
| isLoggedIn = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/router.js` around lines 83 - 90, The guard currently calls
userStore.fetch() regardless of the cached session value, causing an unnecessary
auth API call; change the conditional so the fetch only runs when the session
indicates a logged-in user (use sessionStore.isLoggedIn / the local isLoggedIn
variable) and userStore.data is missing (i.e., only call userStore.fetch() when
isLoggedIn is true and !userStore.data), so you avoid triggering the store's
AuthenticationError and the router.push({ name: 'LoginPage' }) side-effect when
no session exists.
| async function loadChanges() { | ||
| if (!currentChangeRequest.value) return []; | ||
|
|
||
| await changesResource.submit({ | ||
| name: currentChangeRequest.value.name, | ||
| scope: 'summary', | ||
| }); | ||
| return changesResource.data || []; | ||
| if (loadChangesPromise) return loadChangesPromise; | ||
|
|
||
| loadChangesPromise = changesResource | ||
| .submit({ | ||
| name: currentChangeRequest.value.name, | ||
| scope: 'summary', | ||
| }) | ||
| .then(() => changesResource.data || []) | ||
| .finally(() => { | ||
| loadChangesPromise = null; | ||
| }); | ||
|
|
||
| return loadChangesPromise; | ||
| } |
There was a problem hiding this comment.
loadChanges rejection propagates unhandled to callers that don't wrap in try/catch.
loadChanges can reject if the network call fails. Several call sites (SpaceDetails.vue watcher Line 403, refreshTree Line 415) await this without a try/catch, which would result in unhandled promise rejections.
Either add error handling inside loadChanges or ensure all call sites catch errors.
🛡️ Option A: swallow errors inside the store
async function loadChanges() {
if (!currentChangeRequest.value) return [];
if (loadChangesPromise) return loadChangesPromise;
loadChangesPromise = changesResource
.submit({
name: currentChangeRequest.value.name,
scope: 'summary',
})
.then(() => changesResource.data || [])
+ .catch((error) => {
+ console.error('Failed to load changes:', error);
+ return changesResource.data || [];
+ })
.finally(() => {
loadChangesPromise = null;
});
return loadChangesPromise;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/stores/changeRequest.js` around lines 113 - 128, loadChanges
currently lets network rejections bubble up (via changesResource.submit) which
causes unhandled promise rejections at callers; update loadChanges (the async
function loadChanges and its use of loadChangesPromise and
changesResource.submit) to catch errors inside the function, log or record the
error, clear loadChangesPromise in finally as already done, and return a safe
fallback (e.g., []) so callers awaiting loadChanges won't receive a rejected
promise; ensure the catch is applied to the promise chain returned from
changesResource.submit so loadChanges always resolves to an array.
plans/pinia-refactor.md
Outdated
| ``` | ||
| src/stores/ | ||
| index.js # createPinia() instance | ||
| user.js # useUserStore | ||
| session.js # useSessionStore | ||
| changeRequest.js # useChangeRequestStore | ||
| ``` |
There was a problem hiding this comment.
Fenced code block missing language specifier (MD040).
The static analysis tool flagged the code block at Line 13 for not specifying a language.
Suggested fix
-```
+```text
src/stores/
index.js # createPinia() instance📝 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.
| ``` | |
| src/stores/ | |
| index.js # createPinia() instance | |
| user.js # useUserStore | |
| session.js # useSessionStore | |
| changeRequest.js # useChangeRequestStore | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plans/pinia-refactor.md` around lines 13 - 19, The Markdown fenced code block
that lists the src/stores/ tree (the block starting with ``` and containing
"src/stores/ index.js ... changeRequest.js") is missing a language specifier;
update that opening fence to include a plain language token (e.g., ```text or
```bash) so the block conforms to MD040 and static analysis passes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/DraftContributionPanel.vue (1)
114-131:⚠️ Potential issue | 🟠 MajorPrevent stuck loading state and stale draft rendering.
IfcurrentChangeRequestis null,isLoadingnever flips to false and the skeleton can persist forever. Also, when switching drafts,crPagestays populated during the fetch, so old content remains visible.💡 Proposed fix
async function loadCrPage() { if (!crStore.currentChangeRequest) { crPage.value = null; + isLoading.value = false; return; } isLoading.value = true; + crPage.value = null; try { const result = await fetchCrPageResource.submit({ name: crStore.currentChangeRequest.name, doc_key: props.docKey, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DraftContributionPanel.vue` around lines 114 - 131, In loadCrPage, ensure loading state and stale draft are cleared immediately when starting or when no change request is present: if crStore.currentChangeRequest is falsy set isLoading.value = false and crPage.value = null before returning; when beginning a fetch set isLoading.value = true and clear crPage.value = null so old content doesn't remain, then call fetchCrPageResource.submit(...) and assign result to crPage.value inside the try/catch/finally (keeping isLoading.value reset in finally). Reference: function loadCrPage, variables isLoading, crPage, crStore.currentChangeRequest, and fetchCrPageResource.submit.
🧹 Nitpick comments (1)
frontend/src/pages/SpaceDetails.vue (1)
447-454: Avoid duplicateloadChanges()calls after archive/merge.
handleArchiveChangeRequestandhandleMergeChangeRequestcallcrStore.loadChanges()and thenrefreshTree(), which already callscrStore.loadChanges(). This doubles API calls and defeats the PR’s dedup goal.♻️ Suggested change
async function handleArchiveChangeRequest() { try { await crStore.archiveChangeRequest(); toast.success(__('Change request archived')); crStore.currentChangeRequest = null; await crStore.initChangeRequest(props.spaceId); - await crStore.loadChanges(); await refreshTree(); } catch (error) { toast.error(error.messages?.[0] || __('Error archiving change request')); } } async function handleMergeChangeRequest() { const docKey = currentDraftKey.value; try { await crStore.mergeChangeRequest(); toast.success(__('Change request merged')); crStore.currentChangeRequest = null; await crStore.initChangeRequest(props.spaceId); - await crStore.loadChanges(); await refreshTree();Also applies to: 470-478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/SpaceDetails.vue` around lines 447 - 454, Both handleArchiveChangeRequest and handleMergeChangeRequest are calling crStore.loadChanges() and then refreshTree(), but refreshTree() already calls crStore.loadChanges(), causing duplicate API calls; remove the explicit crStore.loadChanges() invocation(s) from handleArchiveChangeRequest and handleMergeChangeRequest so they call refreshTree() (which will perform loadChanges()) after updating crStore.currentChangeRequest and other state, leaving only a single load path via refreshTree().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/WikiDocumentPanel.vue`:
- Around line 125-149: currentCrPage can persist across page/CR context changes,
causing stale draft data to flash; update the watchers and loadCrPage to clear
it: in the watch for props.pageId (where wikiDoc.name is set and
wikiDoc.reload() is called) also reset currentCrPage to null/empty, and in the
combined watch for [() => crStore.currentChangeRequest?.name, () =>
wikiDoc.doc?.doc_key] handle falsy crName or docKey by clearing currentCrPage
immediately (instead of only loading), and inside loadCrPage add an early branch
that when !crStore.currentChangeRequest or !wikiDoc.doc?.doc_key resets
currentCrPage before returning; reference currentCrPage, loadCrPage,
crStore.currentChangeRequest, wikiDoc.doc?.doc_key, and crPageResource.submit
when making these changes.
In `@frontend/src/pages/SpaceDetails.vue`:
- Around line 399-403: The watcher on [() => space.doc, () =>
crStore.isChangeRequestMode, () => crStore.currentChangeRequest?.name] exits
when isChangeRequestMode is false which prevents crTree/treeData from being
populated for non-logged-in users; change the watcher logic so it still builds a
fallback crTree (or forces refreshTree) when !isChangeRequestMode so treeData is
set and the skeleton is removed (use crTree and treeData as the targets). Also
remove the duplicate crStore.loadChanges() calls inside
handleArchiveChangeRequest and handleMergeChangeRequest and rely solely on
refreshTree() to call crStore.loadChanges() so loadChanges() is not executed
twice.
---
Outside diff comments:
In `@frontend/src/components/DraftContributionPanel.vue`:
- Around line 114-131: In loadCrPage, ensure loading state and stale draft are
cleared immediately when starting or when no change request is present: if
crStore.currentChangeRequest is falsy set isLoading.value = false and
crPage.value = null before returning; when beginning a fetch set isLoading.value
= true and clear crPage.value = null so old content doesn't remain, then call
fetchCrPageResource.submit(...) and assign result to crPage.value inside the
try/catch/finally (keeping isLoading.value reset in finally). Reference:
function loadCrPage, variables isLoading, crPage, crStore.currentChangeRequest,
and fetchCrPageResource.submit.
---
Nitpick comments:
In `@frontend/src/pages/SpaceDetails.vue`:
- Around line 447-454: Both handleArchiveChangeRequest and
handleMergeChangeRequest are calling crStore.loadChanges() and then
refreshTree(), but refreshTree() already calls crStore.loadChanges(), causing
duplicate API calls; remove the explicit crStore.loadChanges() invocation(s)
from handleArchiveChangeRequest and handleMergeChangeRequest so they call
refreshTree() (which will perform loadChanges()) after updating
crStore.currentChangeRequest and other state, leaving only a single load path
via refreshTree().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/DraftContributionPanel.vuefrontend/src/components/WikiDocumentPanel.vuefrontend/src/pages/SpaceDetails.vue
| watch( | ||
| [() => space.doc, () => crStore.isChangeRequestMode, () => crStore.currentChangeRequest?.name], | ||
| async ([doc, isMode, crName], oldValues) => { | ||
| if (!doc || !isMode) return; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend && head -c 50000 src/pages/SpaceDetails.vue | wc -lRepository: frappe/wiki
Length of output: 58
🏁 Script executed:
cd frontend && wc -l src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 85
🏁 Script executed:
cd frontend && sed -n '395,420p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 722
🏁 Script executed:
cd frontend && rg -n 'isChangeRequestMode' --type=ts --type=js --type=vue -C3Repository: frappe/wiki
Length of output: 82
🏁 Script executed:
cd frontend && rg -n 'treeData' src/pages/SpaceDetails.vue -C2Repository: frappe/wiki
Length of output: 934
🏁 Script executed:
rg -n 'crStore\.isChangeRequestMode' frontend --type=ts --type=js --type=vue -C2Repository: frappe/wiki
Length of output: 82
🏁 Script executed:
cd frontend && sed -n '280,295p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 564
🏁 Script executed:
cd frontend && sed -n '370,400p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 924
🏁 Script executed:
cd frontend && rg -n 'crTree\s*=' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 90
🏁 Script executed:
cd frontend && rg -n 'handleArchiveChangeRequest\|handleMergeChangeRequest' src/pages/SpaceDetails.vue -A5Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '1,50p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 2199
🏁 Script executed:
cd frontend && rg -n 'useChangeRequestStore' src/pages/SpaceDetails.vue -B2 -A2Repository: frappe/wiki
Length of output: 452
🏁 Script executed:
cd frontend && rg -n 'isChangeRequestMode' frontend/src/stores --type=ts --type=js -B3 -A3Repository: frappe/wiki
Length of output: 114
🏁 Script executed:
cd frontend && rg -n 'skeleton\|Skeleton' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '1,100p' frontend/src/stores/changeRequestStore.tsRepository: frappe/wiki
Length of output: 139
🏁 Script executed:
cd frontend && find . -name 'changeRequest*' -o -name '*changeRequest*'Repository: frappe/wiki
Length of output: 84
🏁 Script executed:
cd frontend && find . -name '*.ts' -path '*/store*' -o -name '*.ts' -path '*/stores*'Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
find . -name 'changeRequestStore*' -type fRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
rg -n 'export.*useChangeRequestStore' frontend --type=tsRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && cat src/stores/changeRequest.jsRepository: frappe/wiki
Length of output: 7195
🏁 Script executed:
cd frontend && sed -n '420,480p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 1870
🏁 Script executed:
cd frontend && rg -n 'loadChanges\|initChangeRequest' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && rg -n 'shouldUseChangeRequestMode' src/stores -A2 -B2Repository: frappe/wiki
Length of output: 806
🏁 Script executed:
cd frontend && rg -n 'SpaceDetails' src --type=ts --type=js -B2 -A2Repository: frappe/wiki
Length of output: 2940
🏁 Script executed:
cd frontend && sed -n '430,460p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 1064
🏁 Script executed:
cd frontend && sed -n '100,150p' src/stores/user.jsRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '340,360p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 528
🏁 Script executed:
cd frontend && rg -n 'refreshTree\|crTree.reload' src/pages/SpaceDetails.vue -B2 -A2Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '1,100p' src/stores/user.jsRepository: frappe/wiki
Length of output: 1581
🏁 Script executed:
cd frontend && rg -n 'WikiDocumentList' src/pages/SpaceDetails.vue -B5 -A5Repository: frappe/wiki
Length of output: 1181
🏁 Script executed:
cd frontend && rg -n 'treeData\|crTree' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && rg -n 'beforeEach\|guard\|auth' src/router.js -B3 -A3Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '250,280p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 912
🏁 Script executed:
cd frontend && cat src/router.js | head -60Repository: frappe/wiki
Length of output: 1387
🏁 Script executed:
cd frontend && rg -n 'currentChangeRequest = null\|isChangeRequestMode' src/stores/changeRequest.jsRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '375,425p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 1354
🏁 Script executed:
cd frontend && grep -n 'v-if.*treeData\|v-if.*skeleton' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 137
🏁 Script executed:
cd frontend && sed -n '35,50p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 774
🏁 Script executed:
cd frontend && rg -n 'Skeleton\|skeleton' src/components/WikiDocumentList.vueRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && head -50 src/components/WikiDocumentList.vueRepository: frappe/wiki
Length of output: 1962
🏁 Script executed:
cd frontend && rg -n 'is_logged_in\|authentication\|auth' src --type=ts --type=js -B2 -A2 | head -50Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cd frontend && sed -n '40,55p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 903
🏁 Script executed:
cd frontend && sed -n '50,70p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 1210
🏁 Script executed:
cd frontend && sed -n '425,445p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 668
🏁 Script executed:
cd frontend && rg -n 'async function refreshTree' src/pages/SpaceDetails.vue -A10Repository: frappe/wiki
Length of output: 365
🏁 Script executed:
cd frontend && sed -n '448,465p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 694
🏁 Script executed:
cd frontend && sed -n '470,490p' src/pages/SpaceDetails.vueRepository: frappe/wiki
Length of output: 812
🏁 Script executed:
cd frontend && rg -n 'isChangeRequestMode\|shouldUseChangeRequestMode' src/components --type=ts --type=js -B2 -A2Repository: frappe/wiki
Length of output: 37
Tree won't populate and skeleton persists for non-logged-in users; remove redundant API calls.
When isChangeRequestMode is false (non-logged-in users), the watcher returns early and crTree never loads, leaving treeData undefined. The sidebar then displays the skeleton loader indefinitely. If non-logged-in users should see the space tree structure, load a fallback tree or ensure CR mode is always enabled for this view.
Additionally, handleArchiveChangeRequest and handleMergeChangeRequest both call crStore.loadChanges() before refreshTree(), which itself calls loadChanges() again. Remove the redundant calls in both handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/SpaceDetails.vue` around lines 399 - 403, The watcher on
[() => space.doc, () => crStore.isChangeRequestMode, () =>
crStore.currentChangeRequest?.name] exits when isChangeRequestMode is false
which prevents crTree/treeData from being populated for non-logged-in users;
change the watcher logic so it still builds a fallback crTree (or forces
refreshTree) when !isChangeRequestMode so treeData is set and the skeleton is
removed (use crTree and treeData as the targets). Also remove the duplicate
crStore.loadChanges() calls inside handleArchiveChangeRequest and
handleMergeChangeRequest and rely solely on refreshTree() to call
crStore.loadChanges() so loadChanges() is not executed twice.
…route names
The stores were pushing/replacing non-existent route names ('LoginPage',
'Login') which silently failed. Use window.location.href consistently
since /login is a server-rendered Frappe page, not a SPA route.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
refreshTree() already calls crStore.loadChanges() internally, so the explicit calls were redundant API requests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
useUserStoreas tracer bullet — validates store pattern, Vue plugin setup, and HMRuseUserStoreand migrate all user-related consumers (isLoggedIn,isWikiManager, etc.)useSessionStore, delete thedata/layer, and move session bootstrap into the storeuseChangeRequestStoreand migrate all CR consumers — centralizes 11 resources, exposes loading getters and action methodsloadCrPage, guard router user fetchuseChangeTypeDisplay,useTreeDialogs)loadChangeswith promise caching, eliminateJSON.parse(JSON.stringify(tree))deep clone viachangeTypeMapprop🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor