feat: Version 3 #431
feat: Version 3 #431
Conversation
|
is this WIP? its great feature based on issue v3. |
|
Any update? |
|
I wanted to do first class git support so went on a de-tour: https://github.com/BuildWithHussain/giki Will move all the above logic to this PR and then continue. Edit: Will be a separate feature added later |
Use separate caption from alt text for better accessibility: -  for accessibility (screen readers) - *caption* on next line for visible caption Changes: - Backend: Remove figure/figcaption, use standard img + em pattern - Frontend: Add caption attribute to image extension with custom marked tokenizer to parse caption pattern - CSS: Style img + em as centered italic caption 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI Agents
In @frontend/src/components/NestedDraggable.vue:
- Line 152: The current global storage key used by
useStorage('wiki-tree-expanded-nodes', {}) causes expansion state to collide
across spaces; change the key to include the current space identifier (e.g., use
a prop or store value like spaceId/currentSpaceId) so expandedNodes is created
with a namespaced key such as `wiki-tree-expanded-nodes-${spaceId}` and update
any references to the expandedNodes variable accordingly; ensure the component
(NestedDraggable.vue) receives or imports the space id (prop, vuex/pinia getter,
or useCurrentSpace) before calling useStorage so each space gets its own storage
entry.
In @frontend/src/components/WikiDocumentList.vue:
- Around line 240-247: The delete_with_children method currently calls
frappe.delete_doc(..., force=True) without an explicit permission check; add a
check at the start of delete_with_children that uses frappe.has_permission("Wiki
Document", "delete", doc=self.name) and if it returns False raise a permission
error via frappe.throw(_("You don't have permission to delete this document"));
keep the check before any destructive calls (frappe.delete_doc) and mirror the
pattern used in delete_wiki_page.
- Line 159: The storage key for expandedNodes is global; namespace it by space
using props.spaceId so each space gets its own key: replace the static key
passed to useStorage('wiki-tree-expanded-nodes', {}) with a reactive/computed
key that includes props.spaceId (e.g., derive key from props.spaceId) and ensure
expandedNodes is reinitialized or switched when props.spaceId changes so the
stored state is per-space; update the useStorage invocation (expandedNodes) to
use that computed key or wrap with a watcher that resets the stored state on
space change.
In @frontend/src/components/WikiEditor.vue:
- Around line 107-126: The paste handler (function handlePaste) currently calls
uploadFile(...) without handling promise rejections; update handlePaste to
handle upload failures by catching errors from uploadFile (either add a
.catch(...) on the returned promise or make handlePaste async and use
try/catch), log the error and surface user feedback (e.g., show a toast/alert or
set an error state) when the upload fails, and ensure you do not call
editor.value.chain().focus().setImage(...) unless uploadFile resolves
successfully.
- Around line 131-155: The handleDrop function currently starts uploads via
uploadFile(file).then(...) but lacks error handling; update the upload call
inside handleDrop (in function handleDrop) to handle failures by adding a .catch
on the promise (or convert to async/await and wrap in try/catch) so any upload
errors are caught and logged or surfaced via the same error handler used for
paste uploads, and ensure editor.value.chain() usage remains inside the success
path only.
- Around line 480-490: The code resets the local isSaving flag with a fragile
setTimeout after emit('save', currentContent); instead, remove the setTimeout
and stop toggling isSaving locally; emit the save and let the parent control a
saving prop; move the logic that clears lastSavedContent.value, sets
hasUnsavedChanges.value and dispatches
document.dispatchEvent('wiki-editor-after-save') into a watcher that reacts to
the saving prop transitioning from true to false (i.e., when saving goes false,
update lastSavedContent.value = currentContent, set hasUnsavedChanges.value =
false, and dispatch the after-save event). Ensure references to isSaving,
emit('save', currentContent), lastSavedContent.value, hasUnsavedChanges.value,
and the watcher on saving prop are updated accordingly.
In @frontend/src/composables/useContributionMode.js:
- Around line 5-6: currentBatch and isLoadingBatch are module-level refs shared
across spaces causing stale batch data when switching spaces; update the spaceId
watcher in components that use this composable (e.g., WikiDocumentPanel,
WikiDocumentList, DraftContributionPanel) to immediately clear the shared state
by setting currentBatch.value = null (and optionally isLoadingBatch.value =
false) at the start of the watcher before fetching the new batch so prior-space
data cannot briefly display or race with the new fetch.
In @frontend/src/pages/ContributionReview.vue:
- Around line 309-363: The four getOperation* functions duplicate switch logic;
replace them with a single configuration object (e.g., OPERATION_CONFIG) that
maps operation keys ('create','edit','delete','move','reorder',etc.) to an
object containing icon, iconClass, theme, and label, then update
getOperationIcon, getOperationIconClass, getOperationTheme, and
getOperationLabel to read from OPERATION_CONFIG and return the corresponding
property or a safe default (fallback icon, classes, theme, or the raw operation
string) so behavior remains identical; keep getStatusTheme unchanged.
- Around line 403-410: The renderMarkdown function currently returns raw HTML
from marked.parse(content), which is unsafe for user input; import DOMPurify and
update renderMarkdown to call DOMPurify.sanitize on the HTML produced by
marked.parse before returning it (keep the existing try/catch and the early
return for falsy content), so v-html consumers (the places using v-html) receive
sanitized HTML and XSS is prevented.
In @frontend/src/pages/Contributions.vue:
- Around line 94-97: The createResource call for pendingReviews uses computed(()
=> isManager.value) for auto; change to the documented pattern by moving the
conditional into makeParams and set auto to true (i.e., implement makeParams()
to return an empty params object when isManager.value is true and return null
when false) so the resource only auto-loads for managers; alternatively, remove
the computed auto and call pendingReviews.fetch() from a watcher on isManager to
trigger fetch when it becomes true.
In @wiki/api/contributions.py:
- Around line 51-65: The loop is doing N+1 DB calls per batch; fix by batching:
collect all batch["wiki_space"] ids, batch["name"] values, and
batch["contributor"] ids first, then run three bulk queries—(1) fetch Wiki Space
names with frappe.get_all("Wiki Space", filters={"name": ["in", space_ids]},
fields=["name","space_name"]), (2) fetch contribution counts grouped by batch
with frappe.get_all or a frappe.db.sql query that SELECTs batch, COUNT(*) AS
contribution_count WHERE batch IN (...) GROUP BY batch, and (3) fetch user info
for all contributors with frappe.get_all("User", filters={"name": ["in",
contributor_ids]}, fields=["name","full_name","user_image"])—then build lookup
dicts and assign batch["wiki_space_name"], batch["contribution_count"],
batch["contributor_name"], and batch["contributor_image"] from those maps
instead of querying inside the for loop (functions/classes to edit: the loop in
wiki/api/contributions.py handling batches).
- Around line 22-26: The current loop over batches causes an N+1 DB pattern by
calling frappe.db.get_value and frappe.db.count per batch; instead, collect all
batch["wiki_space"] and batch["name"] values, fetch space names in one call
(e.g., frappe.get_all/ frappe.db.get_all on "Wiki Space" with filters for the
IN-list) and fetch contribution counts grouped by batch in one grouped query
(e.g., frappe.get_all or frappe.db.get_all on "Wiki Contribution" with
fields=["batch", "count(name) as contribution_count"], group_by="batch"), then
map those results back to each batch and set batch["wiki_space_name"] and
batch["contribution_count"] accordingly.
In @wiki/api/wiki_space.py:
- Around line 14-22: get_descendants_of is called with ignore_permissions=True
which can return private "Wiki Document" records; after retrieving descendants
(the variable descendants used to build _build_wiki_tree_for_api), filter that
list to only include documents the current user can read (e.g., via
frappe.has_permission("Wiki Document", "read", doc) or equivalent permission
check) before passing to _build_wiki_tree_for_api so per-document restrictions
are respected; alternatively remove ignore_permissions=True if safe to rely on
the underlying permission check.
In @wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.json:
- Around line 238-245: The whitelisted functions update_contribution,
delete_contribution, get_contribution_diff, and get_batch_contributions access
Wiki Contribution docs without enforcing document-level ownership; add explicit
checks at the start of each function to validate permission by calling
frappe.throw_if_not_permitted("Wiki Contribution", name) (or loading the doc and
asserting doc.owner == frappe.session.user and throwing) before reading,
modifying, or deleting the document so the if_owner policy is actually enforced
for API calls.
In @wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py:
- Around line 256-289: The create_contribution function currently inserts into
any batch without authorization; before creating the "Wiki Contribution" doc,
load the referenced batch (e.g., via frappe.get_doc("Wiki Batch", batch)) and
enforce that the current user is authorized to add contributions (check
ownership/membership/permissions such as a contributors list, owner field, or
frappe.has_permission for the batch); if the check fails, raise
frappe.PermissionError or frappe.throw to block creation. Ensure this permission
check runs at the top of create_contribution (before frappe.new_doc and
contrib.insert()) and use the batch identifier and current user to make the
decision.
- Around line 292-332: Both update_contribution and delete_contribution lack
authorization checks, so add an explicit permission/ownership check before
modifying or deleting a contribution: load the contribution (contrib) as you
already do, fetch its parent batch/doc (e.g., contrib.parent or
contrib.parent_ref -> frappe.get_doc(...)), then assert the current user can
modify that batch by calling frappe.has_permission(parent_doc, "write") or
comparing parent_doc.owner == frappe.session.user; if the check fails raise
frappe.PermissionError or frappe.throw("Not permitted"); apply the same
pre-check in delete_contribution before calling frappe.delete_doc to prevent
modifying another user's draft batch.
- Around line 361-404: In get_merged_tree, add an explicit permission check for
the provided batch before loading contributions: fetch the batch document (e.g.,
frappe.get_doc("Wiki Contribution Batch", batch)) and call its permission check
(e.g., batch_doc.has_permission("read") or frappe.has_permission("Wiki
Contribution Batch", batch, "read")); if the check fails, raise
frappe.PermissionError or return [] and do not call frappe.get_all("Wiki
Contribution", ...). Place this check right after the early return for no batch
and before the contributions query so unpublished contributions cannot be
leaked.
- Around line 89-98: The auto-generated type hints for the WikiContribution
doctype are missing the original_modified field; update the type hints block
(the class/typing section that lists fields like original_title,
original_content, original_parent, target_route) to include original_modified:
DF.Datetime | None so the declared types match the DocType JSON and the
assignment in snapshot_original().
- Around line 196-199: The _apply_delete() path calls delete_with_children()
which ultimately uses force=True and can bypass permissions; add an explicit
permission check before deletion by loading the target Wiki Document in
_apply_delete(), calling its has_permission("delete") (or using
frappe.has_permission(doc, "delete")) and raising a PermissionError (or
frappe.PermissionError) if the check fails; also audit merge() and apply() to
ensure they cannot be invoked from untrusted Python callers without
document-level delete permission and add the same explicit permission check in
any code paths that reach _apply_delete()/delete_with_children() to guarantee
the caller is authorized to delete the specific Wiki Document.
In @wiki/frappe_wiki/doctype/wiki_document/wiki_document.py:
- Around line 282-302: Add permission checks: before returning child count in
get_children_count, filter the descendants returned by get_descendants_of("Wiki
Document", self.name) to only include names for which
frappe.has_permission("Wiki Document", "read", name) is true (or load each doc
and use doc.has_permission("read")), then return the length of that filtered
list; for delete_with_children, ensure the current user has delete permission on
the root (frappe.has_permission("Wiki Document", "delete", self.name)) and
iterate descendants checking delete permission for each child (or load each with
frappe.get_doc("Wiki Document", name) and call its delete() which enforces
permissions) and remove force=True so permission checks are not bypassed,
raising a PermissionError if any check fails.
In @wiki/public/css/main.css:
- Line 323: Summary: The CSS rule using the :has() selector (.prose img:has(+
em)) is acceptable in modern browsers and requires no change; if you must
support older browsers, add a non-:has() fallback. Fix: keep the existing .prose
img:has(+ em) selector as-is; if legacy support is required, add a fallback
strategy such as marking affected markup with a specific class (e.g., add a
wrapper or a marker class on images with adjacent captions) and write a
corresponding CSS rule targeting that class, or add a small feature-detection
script that adds that class at runtime so the non-:has() CSS applies. Ensure
references to the selector .prose img:has(+ em) remain intact while adding the
alternate selector (the marker class) to handle older browsers.
- Around line 1-4: The CSS in main.css uses Tailwind v4 syntax (@import
"tailwindcss" and @plugin '@tailwindcss/typography') but there is a version
mismatch between package.json (^4.0.14) and frontend/package.json (^3.4.15);
reconcile them by either changing main.css to v3-compatible syntax or updating
frontend/package.json to Tailwind v4 to match package.json, then run the
frontend build to confirm the typography plugin loads correctly; specifically,
decide whether to keep main.css as-is (upgrade frontend/package.json to ^4.x) or
keep frontend on v3 (modify main.css to v3 imports), and update the package
version in frontend/package.json accordingly and re-test the build.
🟡 Minor comments (10)
wiki/public/css/main.css-323-323 (1)
323-323: The:has()selector is widely supported in modern browsers and gracefully degrades.The
:has()pseudo-class (Chrome 105+, Firefox 121+, Safari 15.4+, Edge 105+) is well-established. Even in unsupported browsers, the captions will display correctly—only the tight margin control will not apply. No action needed unless the project explicitly requires support for older browsers.wiki/public/css/main.css-1-4 (1)
1-4: Verify Tailwind CSS version consistency in this project.The
@import "tailwindcss"and@plugin '@tailwindcss/typography'syntax is correct for Tailwind CSS v4, which is now released. However, the repository has a version mismatch:frontend/package.jsonspecifies tailwindcss^3.4.15whilepackage.jsonspecifies^4.0.14. This file uses v4 syntax, so clarify:
- Is this intentional (wiki styling uses v4 while frontend uses v3)?
- If not intentional, update
frontend/package.jsonto use v4.The required CSS dependencies (
font.cssandtheme.css) exist at the expected paths, so those imports will work. Note that Tailwind v4 has some known edge cases with plugin loading—test your build output to ensure the typography plugin loads correctly.frontend/src/components/WikiDocumentList.vue-159-159 (1)
159-159: Namespace expanded nodes storage key per space.The storage key
'wiki-tree-expanded-nodes'is global across all wiki spaces. When switching between spaces, expanded state from one space may incorrectly persist to another. As noted in prior review feedback, this should be namespaced per space.🔎 Suggested fix
-const expandedNodes = useStorage('wiki-tree-expanded-nodes', {}); +const expandedNodes = useStorage(`wiki-tree-expanded-nodes-${props.spaceId}`, {});Note: Since
props.spaceIdis reactive, you may need to use a computed key or handle reactivity appropriately.frontend/src/pages/Contributions.vue-94-97 (1)
94-97: Consider usingmakeParams()instead ofcomputed()for theautoproperty to follow the documented pattern.The
autoproperty in frappe-ui'screateResourceis designed to accept a boolean for controlling auto-fetch behavior. While your pattern withcomputed(() => isManager.value)may work due to Vue 3's reactivity system, the documented and recommended approach for reactive resource parameters is usingmakeParams():const pendingReviews = createResource({ url: 'wiki.api.contributions.get_pending_reviews', makeParams() { return isManager.value ? {} : null; }, auto: true, });Alternatively, if the resource should only fetch when the user is a manager, you could conditionally call fetch:
const pendingReviews = createResource({ url: 'wiki.api.contributions.get_pending_reviews', }); watch(isManager, (newVal) => { if (newVal) pendingReviews.fetch(); });This approach is more explicit, maintainable, and aligns with patterns used throughout the codebase.
frontend/src/components/WikiEditor.vue-107-126 (1)
107-126: Missing error handling for failed image uploads in paste handler.The paste handler calls
uploadFilebut doesn't handle the rejected promise case. If the upload fails, the error is silently swallowed, leaving users confused about why their pasted image didn't appear.🔎 Proposed fix with error handling
function handlePaste(_view, event) { const items = event.clipboardData?.items; if (!items) return false; for (const item of items) { if (item.type.indexOf('image') === 0) { event.preventDefault(); const file = item.getAsFile(); if (file) { - uploadFile(file).then((url) => { - if (editor.value) { - editor.value.chain().focus().setImage({ src: url }).run(); - } - }); + uploadFile(file) + .then((url) => { + if (editor.value) { + editor.value.chain().focus().setImage({ src: url }).run(); + } + }) + .catch((error) => { + console.error('Failed to upload pasted image:', error); + }); } return true; } } return false; }frontend/src/components/WikiEditor.vue-131-155 (1)
131-155: Same issue: Missing error handling for drop uploads.The drop handler also lacks
.catch()for the upload promise. Apply the same fix as for paste handling.🔎 Proposed fix
for (const file of files) { const isImage = file.type.includes('image'); const isVideo = file.type.includes('video'); if (isImage || isVideo) { - uploadFile(file).then((url) => { - if (editor.value) { - if (isVideo) { - editor.value.chain().focus().setVideo({ src: url }).run(); - } else { - editor.value.chain().focus().setImage({ src: url }).run(); + uploadFile(file) + .then((url) => { + if (editor.value) { + if (isVideo) { + editor.value.chain().focus().setVideo({ src: url }).run(); + } else { + editor.value.chain().focus().setImage({ src: url }).run(); + } } - } - }); + }) + .catch((error) => { + console.error('Failed to upload dropped file:', error); + }); } }frontend/src/components/WikiEditor.vue-480-490 (1)
480-490: Race condition:isSavingflag reset via setTimeout can cause issues.The 100ms timeout for resetting
isSavingis fragile. If the parent component's save operation takes longer than 100ms, the watcher might still reset the editor content unexpectedly. Consider using thesavingprop to control this instead of a fixed timeout.🔎 Proposed fix using saving prop
- isSaving = true; emit('save', currentContent); lastSavedContent.value = currentContent; hasUnsavedChanges.value = false; - // Reset flag after a tick to allow the watcher to see the updated props - setTimeout(() => { - isSaving = false; - // Notify components that save is complete so they can restore focus - document.dispatchEvent(new CustomEvent('wiki-editor-after-save')); - }, 100); + // Notify components that save is complete + document.dispatchEvent(new CustomEvent('wiki-editor-after-save'));Then update the watcher to use the
savingprop directly:watch( () => props.content, (newContent) => { - if (isSaving) { + if (props.saving) { return; }Committable suggestion skipped: line range outside the PR's diff.
wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py-361-404 (1)
361-404: Consider permission check for batch access inget_merged_tree.This whitelisted function reads contributions from a batch without verifying that the caller has access to view that batch. If batches contain draft contributions from other users, this could leak unpublished content.
🔎 Suggested fix
@frappe.whitelist() def get_merged_tree(wiki_space: str, batch: str | None = None) -> list: ... + if batch: + batch_doc = frappe.get_doc("Wiki Contribution Batch", batch) + # Ensure user can read this batch (Frappe will check permissions) + batch_doc.check_permission("read") + # Get the space and its root group root_group = frappe.db.get_value("Wiki Space", wiki_space, "root_group")wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py-89-98 (1)
89-98: Update type hints to includeoriginal_modifiedfield.The field
original_modifiedis correctly defined in the DocType JSON as a Datetime field and assigned on line 98, but it's missing from the auto-generated type hints block (lines 18-35). Addoriginal_modified: DF.Datetime | Noneto maintain consistency with other auto-generated field declarations.wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py-196-199 (1)
196-199: Add explicit permission validation for Wiki Document deletion operations.The call chain to
_apply_delete()currently relies on permission checks at the API entry point (merge_batch()validates "Wiki Contribution Batch" write permission). However,delete_with_children()usesforce=Trueonfrappe.delete_doc(), which bypasses standard permission validation. Additionally, themerge()andapply()methods are not@frappe.whitelist()protected and lack internal permission checks, making them callable from Python code paths that may not have validated delete permissions on the target Wiki Document.Ensure that before calling
delete_with_children(), explicit checks confirm the caller has delete permission on the specific Wiki Document being deleted (not just on the batch itself).
🧹 Nitpick comments (41)
wiki/patches.txt (1)
15-15: Clarify intent of the commented patch registration.Line 15 contains a commented patch reference for
assign_wiki_user_to_active_users. This appears to be intentional (deferred work or WIP), but it should be tracked to avoid orphaned comments.Is this a known TODO that should be tracked in an issue, or can it be safely removed? I can help open a GitHub issue to track this deferred migration step if needed.
wiki/public/css/hljs-github-light.css (2)
1-180: Well-structured light theme implementation.The GitHub Light theme is comprehensive and correctly implements highlight.js styling. However, there's an inconsistency: the base
.hljsuses CSS variables (lines 7-8), but all token-specific rules use hardcoded colors.Consider defining all colors as CSS variables for better maintainability and easier theme customization. This would align with the CSS variable pattern started in the base styles.
💡 Example refactor using CSS variables
Define color variables at the top and reference them throughout:
+:root { + --hljs-comment: #6a737d; + --hljs-keyword: #d73a49; + --hljs-string: #032f62; + /* ... other colors */ +} + .hljs { background: var(--surface-gray-1, #f9fafb); color: var(--ink-gray-9, #24292e); } .hljs-comment, .hljs-quote { - color: #6a737d; + color: var(--hljs-comment); font-style: italic; }Then override just the variables for dark theme instead of duplicating all selectors.
181-349: Dark theme implementation is correct.The dark theme properly mirrors the light theme structure with appropriate color adjustments. The use of
[data-theme="dark"]attribute selectors is a solid approach for theme switching.The current implementation works well but duplicates every selector from the light theme. If you adopt the CSS variable approach suggested for the light theme, you could reduce this duplication significantly by only overriding the color variables in the dark theme scope.
wiki/public/css/main.css (1)
47-47: Consider alternatives to!important.Using
!importantcan make styles harder to override or maintain. While this is likely necessary to override the typography plugin's defaults, consider whether Tailwind's layer system or increased specificity could achieve the same result:@layer components { .prose pre { background-color: var(--surface-gray-1); } }If
!importantis required for the plugin override, consider adding a comment explaining why.Also applies to: 56-56
wiki/api/__init__.py (3)
22-22: Consider caching the banner image value.The
banner_imagefrom Website Settings is fetched on everyget_user_info()call. Since this value rarely changes, consider usingfrappe.cacheor fetching it once during boot to improve performance.💡 Example using cache
- "brand_image": frappe.get_single_value("Website Settings", "banner_image"), + "brand_image": frappe.cache.get_value( + "website_banner_image", + generator=lambda: frappe.get_single_value("Website Settings", "banner_image") + ),
27-28: Add return type hint and docstring for consistency.The
get_translations()function is missing a return type hint and docstring, whileget_user_info()includes both. For consistency and better code documentation, consider adding them.💡 Suggested addition
@frappe.whitelist(allow_guest=True) -def get_translations(): +def get_translations() -> dict: + """Get all translations for the user's language.""" if frappe.session.user != "Guest":
30-30: Consider using cached doc for consistency.For consistency with
get_user_info()(line 11) which usesfrappe.get_cached_doc(), you could fetch the User document from cache here as well. This would also make the code more maintainable if additional User fields are needed in the future.💡 Alternative approach
if frappe.session.user != "Guest": - language = frappe.db.get_value("User", frappe.session.user, "language") + user = frappe.get_cached_doc("User", frappe.session.user) + language = user.language else:frontend/src/components/SpaceList.vue (1)
156-165: Rejected promise error message won't surface to the user.When
handleCreateSpacereturnsPromise.reject(new Error("Route is required")), this error won't display in the UI sinceErrorMessageonly showsspaces.insert.error. Consider setting a local error state or usingtoast.error()instead.🔎 Proposed fix
+const localError = ref(''); + const handleCreateSpace = () => { if (!newSpace.route) { - return Promise.reject(new Error("Route is required")); + localError.value = __("Route is required"); + return Promise.resolve(); // Prevent dialog from closing } + localError.value = ''; return spaces.insert.submit({ space_name: newSpace.space_name, route: newSpace.route, }); };Then update the template to show
localError:<ErrorMessage :message="localError || spaces.insert.error" />frontend/src/components/WikiDocumentSettings.vue (1)
93-103: Add client-side validation before saving.The
titleandroutefields are marked asrequiredin the template, butsaveSettings()doesn't validate them before submission. This could result in server-side validation errors that would be better caught early.🔎 Proposed fix
function saveSettings() { + if (!settingsForm.title?.trim() || !settingsForm.route?.trim()) { + toast.error(__('Title and Route are required')); + return; + } + settingsResource.submit({ doctype: 'Wiki Document', name: props.pageId, fieldname: { - title: settingsForm.title, - route: settingsForm.route, + title: settingsForm.title.trim(), + route: settingsForm.route.trim(), is_private: settingsForm.is_private ? 1 : 0, }, }); }frontend/src/components/tiptap-extensions/VideoBlockView.vue (3)
68-76: Redundantsrcattribute on video element.The
<video>element has both asrcattribute and a<source>child element with the same source. When using<source>children, thesrcattribute on<video>is unnecessary and could cause the browser to load the video twice.🔎 Proposed fix
<div v-else class="video-container"> <video - :src="src" controls preload="metadata" class="video-player" > <source :src="src" :type="videoType" /> Your browser does not support the video tag. </video> </div>
17-20: UnusedupdateAttributesprop.The
updateAttributesprop is declared as required but never used in this component. Either remove it if video attributes aren't editable, or implement edit functionality (e.g., changing video source).
138-148: Dark mode may not align with application theme.Using
prefers-color-schememedia query assumes the app follows system preferences. If the wiki has its own theme toggle, consider using CSS custom properties or a theme class on a parent element for consistency.frontend/src/components/tiptap-extensions/WikiToolbar.vue (1)
158-164: Consider adding video upload support alongside image upload.Given the PR includes a
VideoBlockViewcomponent, you might want to add a video upload button to the toolbar for consistency, or document that video insertion is only available via slash commands.frontend/src/components/Sidebar.vue (1)
37-37: Consider namespacing the sidebar collapsed state per space.The storage key
"is-sidebar-collapsed"is global and will be shared across all wiki spaces. If users navigate between different spaces, the collapsed state will be the same for all of them, which may not match user intent.Based on learnings from prior automated review, expanded-node storage was flagged for similar collision issues and should be namespaced per space.
🔎 Proposed fix to namespace the collapsed state
+import { useRoute } from 'vue-router'; + +const route = useRoute(); +const spaceKey = computed(() => route.params.spaceId || 'default'); + -const isSidebarCollapsed = useStorage("is-sidebar-collapsed", true); +const isSidebarCollapsed = useStorage(() => `is-sidebar-collapsed-${spaceKey.value}`, true);frontend/src/pages/ContributionReview.vue (1)
365-401: Consider simplifying the contribution description logic.The
getContributionDescriptionfunction has complex nested logic that could be flattened for better readability.🔎 Proposed simplification
function getContributionDescription(contrib) { + const descriptions = { + create: () => contrib.proposed_is_group + ? __('New group to be created') + : __('New page to be created'), + delete: () => __('Will be deleted'), + move: () => __('Will be moved to a new location'), + reorder: () => __('Position changed'), + edit: () => { + const changes = []; + if (contrib.proposed_title && contrib.proposed_title !== contrib.original_title) { + changes.push(__('title')); + } + if (contrib.proposed_content && contrib.proposed_content !== contrib.original_content) { + changes.push(__('content')); + } + return changes.length > 0 + ? __('Changed: {0}', [changes.join(', ')]) + : __('Content modified'); + } + }; + + return descriptions[contrib.operation]?.() || ''; - switch (contrib.operation) { - case 'create': - return contrib.proposed_is_group - ? __('New group to be created') - : __('New page to be created'); - case 'edit': - const changes = []; - if (contrib.proposed_title && contrib.proposed_title !== contrib.original_title) { - changes.push(__('title')); - } - if (contrib.proposed_content && contrib.proposed_content !== contrib.original_content) { - changes.push(__('content')); - } - return changes.length > 0 - ? __('Changed: {0}', [changes.join(', ')]) - : __('Content modified'); - case 'delete': - return __('Will be deleted'); - case 'move': - return __('Will be moved to a new location'); - case 'reorder': - return __('Position changed'); - default: - return ''; - } }frontend/src/pages/Contributions.vue (2)
147-167: Inconsistent route definition pattern between options objects.
myContributionsOptionsreferences thegetRowRoutefunction, whilereviewsOptionsuses an inline arrow function. For consistency and maintainability, consider using the same pattern for both.🔎 Suggested fix
const reviewsOptions = { selectable: false, showTooltip: true, resizeColumn: false, - getRowRoute: (row) => ({ name: 'ContributionReview', params: { batchId: row.name } }), + getRowRoute: getRowRoute, emptyState: { title: __('No Pending Reviews'), description: __('There are no contributions waiting for review.'), }, };
128-133: Consider extendinggetRowRouteto handle review rows.The current
getRowRouteonly handlesDraftstatus specially. For pending reviews, rows always go toContributionReview. This logic could be unified ingetRowRouteif both tables use the same function.frontend/src/components/tiptap-extensions/SlashCommandsList.vue (1)
23-23: Remove unused imports.
onMountedandonUnmountedare imported but never used in this component.🔎 Suggested fix
-import { ref, watch, onMounted, onUnmounted } from 'vue'; +import { ref, watch } from 'vue';frontend/src/components/tiptap-extensions/callout-block.js (1)
53-80: Consider validating callout type inparseHTML.The
parseHTMLhandlers extract the type from DOM attributes/classes but don't validate againstCALLOUT_TYPES. An invalid type from existing HTML could propagate through the editor.🔎 Suggested fix
{ tag: 'aside.callout', getAttrs: (dom) => { const classList = dom.className.split(' '); const typeClass = classList.find((c) => c.startsWith('callout-')); - const type = typeClass ? typeClass.replace('callout-', '') : 'note'; + let type = typeClass ? typeClass.replace('callout-', '') : 'note'; + if (!CALLOUT_TYPES.includes(type)) { + type = 'note'; + } const titleEl = dom.querySelector('.callout-title span'); const title = titleEl ? titleEl.textContent : ''; const contentEl = dom.querySelector('.callout-content'); const content = contentEl ? contentEl.textContent : ''; return { type, title, content }; }, },frontend/src/components/DraftContributionPanel.vue (4)
141-143: Replaceconsole.errorwith proper error handling.Per PR review feedback,
console.*statements should be replaced with toast notifications or proper logging. The error is caught but only logged to console, which may not be visible to users.🔎 Suggested fix
} catch (error) { - console.error('Error loading contribution:', error); contribution.value = null; + // Error state is handled by the template's else branch showing "Draft not found" } finally {
203-206: Removeconsole.error- error is already displayed via toast.The error is properly surfaced to the user via
toast.error, making theconsole.errorredundant.🔎 Suggested fix
} catch (error) { - console.error('Error saving contribution:', error); toast.error(error.messages?.[0] || __('Error saving draft')); }
238-254: DRY:handleWithdrawBatchandhandleReviseBatchare nearly identical.Both functions call
withdrawBatch()and only differ in the success message. Consider consolidating into a single function with a message parameter.🔎 Suggested fix
-async function handleWithdrawBatch() { +async function handleWithdrawOrRevise(successMessage) { try { await withdrawBatch(); - toast.success(__('Contribution withdrawn')); + toast.success(successMessage); } catch (error) { - toast.error(error.messages?.[0] || __('Error withdrawing contribution')); + toast.error(error.messages?.[0] || __('Error')); } } -async function handleReviseBatch() { - try { - await withdrawBatch(); - toast.success(__('You can now revise your contribution')); - } catch (error) { - toast.error(error.messages?.[0] || __('Error')); - } -} +const handleWithdrawBatch = () => handleWithdrawOrRevise(__('Contribution withdrawn')); +const handleReviseBatch = () => handleWithdrawOrRevise(__('You can now revise your contribution'));
219-223: Removeconsole.error- error is already displayed via toast.Same pattern as above - the error is properly surfaced to users.
🔎 Suggested fix
} catch (error) { - console.error('Error deleting contribution:', error); toast.error(error.messages?.[0] || __('Error deleting draft')); }frontend/src/components/WikiDocumentList.vue (2)
312-315: Removeconsole.error- error is already displayed via toast.Consistent with prior feedback, replace console logging with proper error handling.
🔎 Suggested fix
} catch (error) { - console.error('Error creating contribution:', error); toast.error(error.messages?.[0] || __('Error creating draft')); }
359-362: Removeconsole.error- error is already displayed via toast.Same pattern as above.
🔎 Suggested fix
} catch (error) { - console.error('Error creating delete contribution:', error); toast.error(error.messages?.[0] || __('Error creating draft')); }frontend/src/components/WikiDocumentPanel.vue (1)
293-294: Replaceconsole.errorwith structured logging or remove.Based on prior review feedback,
console.*statements should be replaced with toast notifications or removed. The error is already shown viatoast.erroron line 294, making theconsole.errorredundant for production.Proposed fix
} catch (error) { - console.error('Error saving contribution:', error); toast.error(error.messages?.[0] || __('Error saving draft')); }frontend/src/components/tiptap-extensions/CalloutBlockView.vue (1)
163-200: Hardcoded strings should use i18n translation.The dropdown menu labels ('Edit Title', 'Delete', 'Note', 'Tip', 'Caution', 'Danger') are not wrapped with
__()for internationalization, unlike other components in this PR that consistently use__('...')for user-facing text.Proposed fix
const dropdownOptions = computed(() => [ { - label: 'Edit Title', + label: __('Edit Title'), icon: LucidePencil, onClick: openTitleDialog, }, { - label: 'Delete', + label: __('Delete'), icon: 'trash-2', onClick: () => props.deleteNode(), }, { - group: 'Type', + group: __('Type'), hideLabel: true, items: [ { - label: 'Note', + label: __('Note'), icon: LucideInfo, onClick: () => changeType('note'), }, { - label: 'Tip', + label: __('Tip'), icon: LucideLightbulb, onClick: () => changeType('tip'), }, { - label: 'Caution', + label: __('Caution'), icon: LucideTriangleAlert, onClick: () => changeType('caution'), }, { - label: 'Danger', + label: __('Danger'), icon: LucideShieldAlert, onClick: () => changeType('danger'), }, ], }, ]);Also add the
__import if not already available globally.frontend/src/components/tiptap-extensions/slash-commands.js (1)
9-11: Remove unused imports.
Plugin,PluginKeyfrom@tiptap/pm/stateandDecoration,DecorationSetfrom@tiptap/pm/vieware imported but never used in this file. TheSuggestionplugin handles the ProseMirror plugin creation internally.Proposed fix
import { Extension } from '@tiptap/core'; -import { Plugin, PluginKey } from '@tiptap/pm/state'; -import { Decoration, DecorationSet } from '@tiptap/pm/view'; import Suggestion from '@tiptap/suggestion';frontend/src/composables/useContributionMode.js (1)
46-49: Replaceconsole.errorwith toast notification or structured logging.Per prior review feedback,
console.*statements should be replaced. Consider using a toast notification to inform users of batch loading failures.Proposed fix
+import { toast } from 'frappe-ui'; + // ... in batchResource.onError: onError(error) { - console.error('Failed to get/create batch:', error); + toast.error(__('Failed to load contribution batch')); isLoadingBatch.value = false; },frontend/src/pages/SpaceDetails.vue (2)
160-162: Replace console.error with user-facing error notification.The error is logged to console but not shown to the user. Consider using
toast.error()to inform the user why the setting update failed.🔎 Proposed fix
} catch (error) { - console.error('Failed to update feedback setting:', error); + toast.error(__('Failed to update feedback setting')); enableFeedbackCollection.value = !value;Don't forget to import
toastfromfrappe-ui:-import { createDocumentResource, createResource, Button, Dialog, Switch } from 'frappe-ui'; +import { createDocumentResource, createResource, Button, Dialog, Switch, toast } from 'frappe-ui';
174-333: Consider extracting tree merge logic to a composable.The
mergedTreeDatacomputed property spans 160 lines and handles complex contribution merge logic. While the implementation appears correct, extracting this to a dedicated composable (e.g.,useTreeMerge) would improve testability and maintainability.For example:
// composables/useTreeMerge.js export function useTreeMerge(liveTree, contributions, rootGroup) { // Move merge logic here // Return merged tree } // Then in component: const mergedTreeData = computed(() => { if (!wikiTree.data || !isContributionMode.value) { return wikiTree.data; } return useTreeMerge(wikiTree.data, contributionsResource.data, space.doc.root_group); });wiki/api/contributions.py (1)
98-101: Use consistent datetime function.Line 100 uses
frappe.utils.now()while theapprove()method (shown in relevant snippets) usesnow_datetime(). For consistency and clarity, use the same function throughout.🔎 Proposed fix
+from frappe.utils import now_datetime + batch.status = "Rejected" batch.reviewed_by = frappe.session.user - batch.reviewed_at = frappe.utils.now() + batch.reviewed_at = now_datetime() batch.review_comment = commentfrontend/src/components/tiptap-extensions/ImageNodeView.vue (1)
80-104: Consider adding Tab key handling for better accessibility.The keyboard navigation handles Enter, Escape, and Arrow keys well. Consider adding Tab key handling to allow users to tab out of the caption input naturally, which would improve keyboard accessibility.
🔎 Optional enhancement for Tab key handling
function handleKeydown(event) { const pos = props.getPos(); if (typeof pos !== 'number') return; if (event.key === 'Enter') { event.preventDefault(); // Insert paragraph after image and move cursor there const endPos = pos + props.node.nodeSize; props.editor .chain() .focus() .insertContentAt(endPos, { type: 'paragraph' }) .setTextSelection(endPos + 1) .run(); } else if (event.key === 'Escape' || event.key === 'ArrowDown') { event.preventDefault(); // Move cursor after the image const endPos = pos + props.node.nodeSize; props.editor.chain().focus().setTextSelection(endPos).run(); } else if (event.key === 'ArrowUp') { event.preventDefault(); // Move cursor before the image props.editor.chain().focus().setTextSelection(pos).run(); + } else if (event.key === 'Tab' && !event.shiftKey) { + event.preventDefault(); + // Move to next element after image + const endPos = pos + props.node.nodeSize; + props.editor.chain().focus().setTextSelection(endPos).run(); } }frontend/src/components/WikiEditor.vue (1)
286-299: Potential memory leak: Dynamic import inside onStart may not complete before onExit.The async
import('vue')insideonStartcould complete afteronExitis called if the user quickly dismisses the slash command popup. AlthoughisDestroyedcheck exists, the timing could still cause issues. Consider using a static import since Vue is already imported at the top of the file.🔎 Proposed fix - use existing Vue import
onStart: (props) => { isDestroyed = false; // Create a container for the Vue component const container = document.createElement('div'); // Create the Vue component instance component = { element: container, props, vm: null, app: null, }; // Mount the SlashCommandsList component - import('vue').then(({ createApp, h }) => { - if (isDestroyed) return; - const app = createApp({ - render() { - return h(SlashCommandsList, { - items: props.items, - command: props.command, - ref: 'listRef', - }); - }, - }); - component.app = app; - component.vm = app.mount(container); + // Use already-imported createApp and h from line 25 + const app = createApp({ + render() { + return h(SlashCommandsList, { + items: props.items, + command: props.command, + ref: 'listRef', + }); + }, }); + component.app = app; + component.vm = app.mount(container);wiki/frappe_wiki/doctype/wiki_contribution_batch/wiki_contribution_batch.py (2)
140-172: Consider wrapping merge operations in a transaction for atomicity.The
merge()method applies multiple contributions and then rebuilds the NSM tree. If an error occurs mid-way (e.g., during one contribution's apply), the batch status won't be updated but some documents may already be modified. Consider wrapping this in a savepoint or ensuring proper rollback semantics.🔎 Proposed enhancement with error handling
def merge(self): """ Merge all approved contributions into the live Wiki Document tree. This is the main operation that applies all changes. """ if self.status != "Approved": frappe.throw(_("Only approved batches can be merged")) contributions = self.get_contributions() if not contributions: frappe.throw(_("No contributions to merge")) # Map temp_ids to real document names as we create them temp_to_real = {} - for contrib in contributions: - contrib_doc = frappe.get_doc("Wiki Contribution", contrib.name) - contrib_doc.apply(temp_to_real) + try: + for contrib in contributions: + contrib_doc = frappe.get_doc("Wiki Contribution", contrib.name) + contrib_doc.apply(temp_to_real) + except Exception: + frappe.db.rollback() + raise # Mark batch as merged self.status = "Merged" self.save()
235-243: Permission check uses generic "write" instead of specific action.For
approve_batchandreject_batch, the permission check uses"write"permission. Consider whether a more specific permission (like a custom "approve" or "review" permission) would provide finer-grained access control, distinguishing reviewers from regular editors.wiki/frappe_wiki/doctype/wiki_contribution/test_wiki_contribution.py (1)
462-475: Consider adding uniqueness to space_name for better test isolation.The
create_test_wiki_space()helper uses a random hash for the route and root_group title, butspace_nameis always "Test Space". If tests run in parallel or don't fully rollback, this could cause conflicts.🔎 Proposed fix
def create_test_wiki_space(): """Create a test Wiki Space with a root group.""" + unique_suffix = frappe.generate_hash(length=6) root_group = frappe.new_doc("Wiki Document") - root_group.title = f"Test Root {frappe.generate_hash(length=6)}" + root_group.title = f"Test Root {unique_suffix}" root_group.is_group = 1 root_group.insert() space = frappe.new_doc("Wiki Space") - space.space_name = "Test Space" - space.route = f"test-space-{frappe.generate_hash(length=6)}" + space.space_name = f"Test Space {unique_suffix}" + space.route = f"test-space-{unique_suffix}" space.root_group = root_group.name space.insert() return spacewiki/frappe_wiki/doctype/wiki_document/wiki_document.py (2)
92-136: Complex route building logic - consider extracting to a helper.The
set_routemethod has nested logic for handling new vs existing documents, ancestor traversal, and route construction. While functional, extracting the ancestor chain building into a separate helper would improve readability and testability.
328-336: Redirect logic could be cleaner with early return.The redirect inside the loop after finding the first published child works, but
frappe.redirect()raises an exception to halt execution. The code after the loop won't execute once a redirect happens, but this isn't immediately obvious.🔎 Proposed clarification
# Redirect to first published child document if available if root_group: child_docs = get_descendants_of( "Wiki Document", root_group, order_by="lft asc, sort_order desc", ignore_permissions=True ) for child_name in child_docs: child_doc = frappe.get_cached_doc("Wiki Document", child_name) if not child_doc.is_group and child_doc.is_published: frappe.redirect("/" + child_doc.route) + # frappe.redirect raises Redirect exception, so this line is never reached + return False - return False + return Falsewiki/frappe_wiki/doctype/wiki_document/test_wiki_document.py (2)
20-26: Empty test class - consider populating or removing.
IntegrationTestWikiDocumentis currently a placeholder with onlypass. Consider adding integration tests or removing if not needed.
35-52: Class-leveltest_docs/test_spacesmay leak state between test methods.
setUpClassinitializescls.test_docsandcls.test_spacesat the class level, buttearDownresets them as instance attributes withself.test_docs = []. This means documents from one test method could remain in the class-level list iftearDownfails or if the instance attribute shadows the class attribute.Consider using
setUpinstead ofsetUpClass, or consistently using class-level operations:🔎 Suggested fix using setUp
- @classmethod - def setUpClass(cls): - super().setUpClass() - cls.test_docs = [] - cls.test_spaces = [] + def setUp(self): + super().setUp() + self.test_docs = [] + self.test_spaces = []
| localItems.value = [...newItems]; | ||
| }, { deep: true }); | ||
|
|
||
| const expandedNodes = useStorage('wiki-tree-expanded-nodes', {}); |
There was a problem hiding this comment.
Namespace localStorage key per space to avoid state collision.
The expanded nodes state uses a global key 'wiki-tree-expanded-nodes' shared across all spaces. This causes the expansion state of one space to affect others.
🔎 Proposed fix
-const expandedNodes = useStorage('wiki-tree-expanded-nodes', {});
+const expandedNodes = useStorage(`wiki-tree-expanded-nodes-${props.spaceId}`, {});This ensures each space maintains its own independent expansion state.
📝 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.
| const expandedNodes = useStorage('wiki-tree-expanded-nodes', {}); | |
| const expandedNodes = useStorage(`wiki-tree-expanded-nodes-${props.spaceId}`, {}); |
🤖 Prompt for AI Agents
In @frontend/src/components/NestedDraggable.vue around line 152, The current
global storage key used by useStorage('wiki-tree-expanded-nodes', {}) causes
expansion state to collide across spaces; change the key to include the current
space identifier (e.g., use a prop or store value like spaceId/currentSpaceId)
so expandedNodes is created with a namespaced key such as
`wiki-tree-expanded-nodes-${spaceId}` and update any references to the
expandedNodes variable accordingly; ensure the component (NestedDraggable.vue)
receives or imports the space id (prop, vuex/pinia getter, or useCurrentSpace)
before calling useStorage so each space gets its own storage entry.
| deleteDocResource.value = createDocumentResource({ | ||
| doctype: 'Wiki Document', | ||
| name: node.name, | ||
| whitelistedMethods: { | ||
| getChildrenCount: 'get_children_count', | ||
| deleteWithChildren: 'delete_with_children', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for delete_with_children implementation in the codebase
rg -n 'def delete_with_children' --type py -A 15Repository: frappe/wiki
Length of output: 166
🏁 Script executed:
# Also search for any permission checks related to wiki document deletion
rg -n 'has_permission|can_delete' --type py -B 2 -A 5 | grep -i 'wiki\|delete_with_children' -A 5 -B 5Repository: frappe/wiki
Length of output: 13499
🏁 Script executed:
# Find the Wiki Document model/doctype file
fd -e 'py' -type f | xargs grep -l 'delete_with_children' | head -20Repository: frappe/wiki
Length of output: 225
🏁 Script executed:
# Search for delete_with_children in all Python files
rg -n 'delete_with_children' --type py -B 5 -A 15Repository: frappe/wiki
Length of output: 4120
🏁 Script executed:
# Look for Wiki Document doctype Python file
fd 'wiki_document' -e py | head -20Repository: frappe/wiki
Length of output: 171
🏁 Script executed:
# Check Wiki Document doctype configuration
fd 'wiki_document' -e json | head -20Repository: frappe/wiki
Length of output: 112
🏁 Script executed:
# Look for doctype JSON file or permission configuration
rg -n 'Wiki Document' wiki/frappe_wiki/doctype/wiki_document/ -A 5 | grep -i 'permission\|role' -B 2 -A 2Repository: frappe/wiki
Length of output: 1856
🏁 Script executed:
# Check the test to understand what permissions are expected
rg -n 'test_wiki_manager_can_delete_document' -A 20Repository: frappe/wiki
Length of output: 1095
🏁 Script executed:
# Get the full permissions section from wiki_document.json
cat wiki/frappe_wiki/doctype/wiki_document/wiki_document.json | jq '.permissions'Repository: frappe/wiki
Length of output: 572
🏁 Script executed:
# Also check if there are any permission rules or before_delete hooks
rg -n 'before_delete|permission' wiki/frappe_wiki/doctype/wiki_document/wiki_document.py -A 3 -B 1Repository: frappe/wiki
Length of output: 745
🏁 Script executed:
# Check if Frappe's whitelist decorator automatically enforces doctype permissions
rg -n '@frappe.whitelist' wiki/frappe_wiki/doctype/wiki_document/wiki_document.py -A 5 | head -30Repository: frappe/wiki
Length of output: 1247
🏁 Script executed:
# Compare with other whitelisted methods that handle destructive operations
rg -n '@frappe.whitelist' wiki/ -A 8 | grep -B 8 'frappe.has_permission.*delete' | head -40Repository: frappe/wiki
Length of output: 731
Add explicit permission check to delete_with_children method before deletion.
The method at lines 289-302 executes a destructive operation with frappe.delete_doc(..., force=True) but lacks explicit permission validation. While doctype permissions exist (Wiki Manager role has delete access), the method should include a direct permission check before proceeding, consistent with other destructive operations in the codebase (e.g., delete_wiki_page in wiki_page.py explicitly validates with frappe.has_permission(..., "delete")). Add a permission check at the start of the method:
if not frappe.has_permission("Wiki Document", "delete", doc=self.name):
frappe.throw(_("You don't have permission to delete this document"))🤖 Prompt for AI Agents
In @frontend/src/components/WikiDocumentList.vue around lines 240-247, The
delete_with_children method currently calls frappe.delete_doc(..., force=True)
without an explicit permission check; add a check at the start of
delete_with_children that uses frappe.has_permission("Wiki Document", "delete",
doc=self.name) and if it returns False raise a permission error via
frappe.throw(_("You don't have permission to delete this document")); keep the
check before any destructive calls (frappe.delete_doc) and mirror the pattern
used in delete_wiki_page.
| const currentBatch = ref(null); | ||
| const isLoadingBatch = ref(false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the file and understand its structure
echo "=== Locating useContributionMode.js ==="
fd useContributionMode.js
echo -e "\n=== Checking full composable file ==="
cat -n frontend/src/composables/useContributionMode.js | head -50
echo -e "\n=== Searching for useContributionMode usage ==="
rg -n "useContributionMode" --type=vue --type=js -B 2 -A 8Repository: frappe/wiki
Length of output: 1986
🏁 Script executed:
#!/bin/bash
echo "=== Full useContributionMode.js file ==="
cat -n frontend/src/composables/useContributionMode.js
echo -e "\n=== Searching for useContributionMode usage in all files ==="
rg -n "useContributionMode" -A 10 | head -100
echo -e "\n=== Checking for spaceId watchers in components ==="
rg -n "watch.*spaceId" -A 5Repository: frappe/wiki
Length of output: 14598
Clear currentBatch when the wiki space changes to prevent stale data.
currentBatch and isLoadingBatch are module-level refs shared across all spaces. When a user switches between spaces, the old batch persists until the new batch is fetched, creating a race condition where components may briefly display the previous space's batch. Add currentBatch.value = null; at the start of the spaceId watcher in components using this composable (e.g., WikiDocumentPanel, WikiDocumentList, DraftContributionPanel).
🤖 Prompt for AI Agents
In @frontend/src/composables/useContributionMode.js around lines 5-6,
currentBatch and isLoadingBatch are module-level refs shared across spaces
causing stale batch data when switching spaces; update the spaceId watcher in
components that use this composable (e.g., WikiDocumentPanel, WikiDocumentList,
DraftContributionPanel) to immediately clear the shared state by setting
currentBatch.value = null (and optionally isLoadingBatch.value = false) at the
start of the watcher before fetching the new batch so prior-space data cannot
briefly display or race with the new fetch.
| function getStatusTheme(status) { | ||
| switch (status) { | ||
| case 'Draft': return 'blue'; | ||
| case 'Submitted': return 'orange'; | ||
| case 'Under Review': return 'orange'; | ||
| case 'Approved': return 'green'; | ||
| case 'Rejected': return 'red'; | ||
| case 'Merged': return 'green'; | ||
| default: return 'gray'; | ||
| } | ||
| } | ||
|
|
||
| function getOperationIcon(operation) { | ||
| switch (operation) { | ||
| case 'create': return LucidePlus; | ||
| case 'edit': return LucidePencil; | ||
| case 'delete': return LucideTrash2; | ||
| case 'move': return LucideMove; | ||
| case 'reorder': return LucideArrowUpDown; | ||
| default: return LucideFileText; | ||
| } | ||
| } | ||
|
|
||
| function getOperationIconClass(operation) { | ||
| switch (operation) { | ||
| case 'create': return 'bg-green-100 text-green-600'; | ||
| case 'edit': return 'bg-blue-100 text-blue-600'; | ||
| case 'delete': return 'bg-red-100 text-red-600'; | ||
| case 'move': return 'bg-purple-100 text-purple-600'; | ||
| case 'reorder': return 'bg-gray-100 text-gray-600'; | ||
| default: return 'bg-gray-100 text-gray-600'; | ||
| } | ||
| } | ||
|
|
||
| function getOperationTheme(operation) { | ||
| switch (operation) { | ||
| case 'create': return 'green'; | ||
| case 'edit': return 'blue'; | ||
| case 'delete': return 'red'; | ||
| case 'move': return 'purple'; | ||
| case 'reorder': return 'gray'; | ||
| default: return 'gray'; | ||
| } | ||
| } | ||
|
|
||
| function getOperationLabel(operation) { | ||
| switch (operation) { | ||
| case 'create': return __('New'); | ||
| case 'edit': return __('Edit'); | ||
| case 'delete': return __('Delete'); | ||
| case 'move': return __('Move'); | ||
| case 'reorder': return __('Reorder'); | ||
| default: return operation; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicate switch statements into a configuration object.
Four similar switch statements mapping operation types to UI properties create significant code duplication. This violates the DRY principle and makes maintenance harder.
🔎 Proposed refactor using a configuration object
+const OPERATION_CONFIG = {
+ create: {
+ icon: LucidePlus,
+ iconClass: 'bg-green-100 text-green-600',
+ theme: 'green',
+ label: () => __('New')
+ },
+ edit: {
+ icon: LucidePencil,
+ iconClass: 'bg-blue-100 text-blue-600',
+ theme: 'blue',
+ label: () => __('Edit')
+ },
+ delete: {
+ icon: LucideTrash2,
+ iconClass: 'bg-red-100 text-red-600',
+ theme: 'red',
+ label: () => __('Delete')
+ },
+ move: {
+ icon: LucideMove,
+ iconClass: 'bg-purple-100 text-purple-600',
+ theme: 'purple',
+ label: () => __('Move')
+ },
+ reorder: {
+ icon: LucideArrowUpDown,
+ iconClass: 'bg-gray-100 text-gray-600',
+ theme: 'gray',
+ label: () => __('Reorder')
+ }
+};
+
+const DEFAULT_OPERATION_CONFIG = {
+ icon: LucideFileText,
+ iconClass: 'bg-gray-100 text-gray-600',
+ theme: 'gray',
+ label: (op) => op
+};
+
function getOperationIcon(operation) {
- switch (operation) {
- case 'create': return LucidePlus;
- case 'edit': return LucidePencil;
- case 'delete': return LucideTrash2;
- case 'move': return LucideMove;
- case 'reorder': return LucideArrowUpDown;
- default: return LucideFileText;
- }
+ return (OPERATION_CONFIG[operation] || DEFAULT_OPERATION_CONFIG).icon;
}
function getOperationIconClass(operation) {
- switch (operation) {
- case 'create': return 'bg-green-100 text-green-600';
- case 'edit': return 'bg-blue-100 text-blue-600';
- case 'delete': return 'bg-red-100 text-red-600';
- case 'move': return 'bg-purple-100 text-purple-600';
- case 'reorder': return 'bg-gray-100 text-gray-600';
- default: return 'bg-gray-100 text-gray-600';
- }
+ return (OPERATION_CONFIG[operation] || DEFAULT_OPERATION_CONFIG).iconClass;
}
function getOperationTheme(operation) {
- switch (operation) {
- case 'create': return 'green';
- case 'edit': return 'blue';
- case 'delete': return 'red';
- case 'move': return 'purple';
- case 'reorder': return 'gray';
- default: return 'gray';
- }
+ return (OPERATION_CONFIG[operation] || DEFAULT_OPERATION_CONFIG).theme;
}
function getOperationLabel(operation) {
- switch (operation) {
- case 'create': return __('New');
- case 'edit': return __('Edit');
- case 'delete': return __('Delete');
- case 'move': return __('Move');
- case 'reorder': return __('Reorder');
- default: return operation;
- }
+ const config = OPERATION_CONFIG[operation] || DEFAULT_OPERATION_CONFIG;
+ return typeof config.label === 'function' ? config.label(operation) : config.label;
}🤖 Prompt for AI Agents
In @frontend/src/pages/ContributionReview.vue around lines 309-363, The four
getOperation* functions duplicate switch logic; replace them with a single
configuration object (e.g., OPERATION_CONFIG) that maps operation keys
('create','edit','delete','move','reorder',etc.) to an object containing icon,
iconClass, theme, and label, then update getOperationIcon,
getOperationIconClass, getOperationTheme, and getOperationLabel to read from
OPERATION_CONFIG and return the corresponding property or a safe default
(fallback icon, classes, theme, or the raw operation string) so behavior remains
identical; keep getStatusTheme unchanged.
| function renderMarkdown(content) { | ||
| if (!content) return ''; | ||
| try { | ||
| return marked.parse(content); | ||
| } catch (e) { | ||
| return content; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -o -name "package-lock.json" | head -20Repository: frappe/wiki
Length of output: 93
🏁 Script executed:
grep -r "marked" --include="package.json" --include="package-lock.json" -A 2 -B 2Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat -n frontend/src/pages/ContributionReview.vue | sed -n '118p;124p;134p;403p;404p;405p;406p;407p;408p;409p;410p'Repository: frappe/wiki
Length of output: 492
🏁 Script executed:
cat frontend/package.json | grep -A 5 -B 5 "marked"Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat package.json | grep -A 5 -B 5 "marked"Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat frontend/package.json | grep -i "dompurify\|sanitize\|xss"Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
rg "DOMPurify|sanitize|dompurify" frontend/src/ -iRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat frontend/package.jsonRepository: frappe/wiki
Length of output: 1639
🏁 Script executed:
cat package.jsonRepository: frappe/wiki
Length of output: 757
🏁 Script executed:
head -50 frontend/src/pages/ContributionReview.vue | grep -i "import.*marked\|marked"Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
rg "marked" frontend/src/ --type vue --type jsRepository: frappe/wiki
Length of output: 82
🏁 Script executed:
head -100 frontend/src/pages/ContributionReview.vue | cat -nRepository: frappe/wiki
Length of output: 4465
🏁 Script executed:
rg "import.*marked\|from.*marked" --type js --type vueRepository: frappe/wiki
Length of output: 82
🏁 Script executed:
rg "marked" frontend/src/Repository: frappe/wiki
Length of output: 475
🏁 Script executed:
cat frontend/src/pages/ContributionReview.vue | head -300 | tail -100Repository: frappe/wiki
Length of output: 2807
🏁 Script executed:
rg "from.*frappe-ui\|import.*frappe-ui" frontend/src/pages/ContributionReview.vueRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
grep -A 5 "^marked@" frontend/yarn.lock 2>/dev/null || grep -A 5 '"marked"' frontend/yarn.lock 2>/dev/null | head -20Repository: frappe/wiki
Length of output: 325
🏁 Script executed:
grep -A 5 "^marked@" frontend/package-lock.json 2>/dev/null || grep -A 5 '"marked"' frontend/package-lock.json 2>/dev/null | head -20Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
ls frontend/*.lock frontend/*.json 2>/dev/nullRepository: frappe/wiki
Length of output: 95
🌐 Web query:
marked.js version 15 XSS sanitization HTML security
💡 Result:
Short answer: marked v15 does NOT sanitize HTML for you — it’s explicitly unsafe-by-default. You must sanitize the HTML output yourself (DOMPurify, sanitize‑html, etc.) or use a wrapper that does so. (npmjs.com)
Key points and recommendations:
- The official marked README warns: "Marked does not sanitize the output HTML. Please use a sanitize library, like DOMPurify (recommended)…". (npmjs.com)
- Historically marked had a deprecated sanitize option and has had XSS-related vulnerabilities; relying on marked for sanitization is unsafe. Use a dedicated sanitizer after marked.parse/marked.parseInline. (snyk.io)
- Practical options:
- DOMPurify (client/server) to sanitize marked’s HTML output. (npmjs.com)
- sanitize-html or js‑xss as alternatives. (npmjs.com)
- Use community wrappers that combine marked + DOMPurify (e.g., safe-marked) or GitHub-style sanitizers if you need GitHub-like sanitization behavior. (github.com)
Minimal safe pattern (conceptual):
- html = marked.parse(md)
- safe = DOMPurify.sanitize(html)
If you want, I can show example code for Node/browser with DOMPurify or evaluate your current implementation for XSS risks.
Citations:
- 1: https://www.npmjs.com/package/marked/v/15.0.10?utm_source=openai
- 2: https://www.npmjs.com/package/marked/v/15.0.10?utm_source=openai
- 3: https://snyk.io/blog/marked-xss-vulnerability/?utm_source=openai
- 4: https://www.npmjs.com/package/marked/v/15.0.10?utm_source=openai
- 5: https://www.npmjs.com/package/xss/v/0.1.15?utm_source=openai
- 6: https://github.com/azu/safe-marked?utm_source=openai
🌐 Web query:
marked library parse method sanitize HTML output security
💡 Result:
Short answer: marked.parse does NOT sanitize — always sanitize the HTML it returns (e.g. with DOMPurify or sanitize-html) before inserting into the DOM or storing for later rendering. [1][2]
Why and how (concise):
- marked explicitly warns that it does not sanitize output and that its old sanitize option was/ will be removed; treating marked.parse output as unsafe can lead to XSS. Use a dedicated sanitizer as a second step. [1][2]
- Common safe pattern (browser):
- const html = marked.parse(markdown);
- const safe = DOMPurify.sanitize(html);
- container.innerHTML = safe;
Use sanitize-html or DOMPurify on the server. [1][3]
- Consider additional hardening: configure the sanitizer allowlist/transform rules (e.g. strip javascript: URLs, disallow on* attributes, limit iframes), use CSP and Trusted Types in the browser, and keep marked and your sanitizer up to date. [1][2][3]
- Historical note: marked had past XSS issues (see CVE) in older versions — upgrade to a maintained version and follow the sanitize step. [4]
References:
[1] marked documentation / repo (explicit warning).
[2] marked npm / README (sanitize warning).
[3] safe-marked (example integration: marked + DOMPurify).
[4] CVE-2017-1000427 (past XSS in old marked versions).
Add HTML sanitization using DOMPurify to prevent XSS vulnerability.
The marked.parse() function (version 15.0.12) explicitly does not sanitize HTML output. Since the parsed markdown is rendered via v-html at lines 118, 124, and 134 with user-provided content, this creates a critical XSS vulnerability. Install dompurify and update the renderMarkdown() function to sanitize the output before rendering:
import DOMPurify from 'dompurify';
function renderMarkdown(content) {
if (!content) return '';
try {
const html = marked.parse(content);
return DOMPurify.sanitize(html);
} catch (e) {
return content;
}
}🤖 Prompt for AI Agents
In @frontend/src/pages/ContributionReview.vue around lines 403-410, The
renderMarkdown function currently returns raw HTML from marked.parse(content),
which is unsafe for user input; import DOMPurify and update renderMarkdown to
call DOMPurify.sanitize on the HTML produced by marked.parse before returning it
(keep the existing try/catch and the early return for falsy content), so v-html
consumers (the places using v-html) receive sanitized HTML and XSS is prevented.
|
|
||
| root_group = space.root_group | ||
| descendants = get_descendants_of("Wiki Document", root_group, ignore_permissions=True) | ||
|
|
||
| if not descendants: | ||
| return {"children": [], "root_group": root_group} | ||
|
|
||
| tree = _build_wiki_tree_for_api(descendants) | ||
| return {"children": tree, "root_group": root_group} |
There was a problem hiding this comment.
get_descendants_of with ignore_permissions=True may expose private documents.
While the space read permission is checked, ignore_permissions=True on Line 16 means all descendant documents are returned regardless of per-document permissions. If some documents are private or have restricted access, they could be exposed in the tree response.
🔎 Proposed fix
Consider filtering the documents after retrieval based on document-level permissions, or adding a note in the docstring about this behavior:
def get_wiki_tree(space_id: str) -> dict:
- """Get the tree structure of Wiki Documents for a given Wiki Space."""
+ """Get the tree structure of Wiki Documents for a given Wiki Space.
+
+ Note: Returns all documents in the space if user has space read permission.
+ Document-level privacy is handled at the frontend/render level.
+ """
space = frappe.get_cached_doc("Wiki Space", space_id)
space.check_permission("read")Or filter documents:
descendants = get_descendants_of("Wiki Document", root_group, ignore_permissions=True)
# Filter to only include documents user can read
if descendants:
descendants = [d for d in descendants if frappe.has_permission("Wiki Document", "read", d)]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @wiki/api/wiki_space.py around lines 14-22, get_descendants_of is called with
ignore_permissions=True which can return private "Wiki Document" records; after
retrieving descendants (the variable descendants used to build
_build_wiki_tree_for_api), filter that list to only include documents the
current user can read (e.g., via frappe.has_permission("Wiki Document", "read",
doc) or equivalent permission check) before passing to _build_wiki_tree_for_api
so per-document restrictions are respected; alternatively remove
ignore_permissions=True if safe to rely on the underlying permission check.
| { | ||
| "create": 1, | ||
| "delete": 1, | ||
| "if_owner": 1, | ||
| "read": 1, | ||
| "role": "All", | ||
| "write": 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "wiki_contribution.py" -x wc -l {}Repository: frappe/wiki
Length of output: 199
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | head -100Repository: frappe/wiki
Length of output: 4303
🏁 Script executed:
grep -n -E "(frappe.whitelist|has_permission|is_owner|frappe.only_for|@frappe)" ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.pyRepository: frappe/wiki
Length of output: 198
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '250,370p'Repository: frappe/wiki
Length of output: 4458
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '256n-290p'Repository: frappe/wiki
Length of output: 116
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '256,290p'Repository: frappe/wiki
Length of output: 1421
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '292,332p'Repository: frappe/wiki
Length of output: 1665
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '328,352p'Repository: frappe/wiki
Length of output: 887
🏁 Script executed:
grep -n "def has_permission\|def is_owner" ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.pyRepository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '1,250p' | grep -A 5 -B 5 "permission\|owner"Repository: frappe/wiki
Length of output: 37
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | sed -n '100,250p'Repository: frappe/wiki
Length of output: 5868
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.py | tail -20Repository: frappe/wiki
Length of output: 786
🏁 Script executed:
grep -n "owner\|permission" ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.pyRepository: frappe/wiki
Length of output: 146
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.json | sed -n '1,50p'Repository: frappe/wiki
Length of output: 1411
🏁 Script executed:
cat -n ./wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.json | sed -n '210,251p'Repository: frappe/wiki
Length of output: 991
🏁 Script executed:
grep -rn "if_owner" ./wiki/frappe_wiki/doctype/ --include="*.py"Repository: frappe/wiki
Length of output: 37
Add explicit ownership checks to all whitelisted API methods.
The whitelisted methods update_contribution(), delete_contribution(), get_contribution_diff(), and get_batch_contributions() access Wiki Contribution documents without explicit ownership validation. While the permission schema declares if_owner: 1 for the "All" role, module-level whitelisted functions do not automatically enforce document-level permission checks. Add explicit checks like frappe.throw_if_not_permitted("Wiki Contribution", name) or compare doc.owner == frappe.session.user before allowing modifications or retrievals.
🤖 Prompt for AI Agents
In @wiki/frappe_wiki/doctype/wiki_contribution/wiki_contribution.json around
lines 238-245, The whitelisted functions update_contribution,
delete_contribution, get_contribution_diff, and get_batch_contributions access
Wiki Contribution docs without enforcing document-level ownership; add explicit
checks at the start of each function to validate permission by calling
frappe.throw_if_not_permitted("Wiki Contribution", name) (or loading the doc and
asserting doc.owner == frappe.session.user and throwing) before reading,
modifying, or deleting the document so the if_owner policy is actually enforced
for API calls.
|
YOLO ✌🏼 |
Closes #451 #459 #456 #442 #438 #430 #411 #397 #310
ToDo:
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.