Wik v2 pages tree #34
Conversation
📝 WalkthroughWalkthroughThis pull request implements a major architectural shift from slug-based to path-based page hierarchy. It introduces a Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes The scale encompasses the entire application stack: database schema and migrations, backend resource definitions with complex path operations, comprehensive frontend JavaScript/CSS for editor and tree UI, and extensive LiveView refactoring across multiple pages. Logic density is high (tree operations, path normalization, subtree moves, wikilink translation), file heterogeneity is maximal (Elixir business logic, JavaScript hooks, CSS styling, templates), and interconnected changes require understanding multiple subsystems simultaneously. The refactoring touches core navigation, state management, and editor integration. Possibly related PRs
Poem
🚥 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. ✨ Finishing touches
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 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/wik_web/live/tag_live/show.ex (1)
79-92: Fix handling whenpage_pathisnilto prevent malformed navigation links.
page_tree_path_forreturnsnilwhen the page isn't found in the tree. When this happens,page_urlfalls back to returning"#", and the anchor concatenation on line 92 (base_url <> "##{slug}") produces a malformed link with double##("###slug").Either guard against nil:
<% page_path = page_tree_path_for(`@ctx`, block.page.id) %> <% base_url = if page_path, do: WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path), else: "#" %>Or pass page_path directly to page_url and let it handle nil by returning
"#"(then anchor concatenation becomes"###{slug}", still malformed—so the guard is necessary).lib/wik_web/live/group_live/wikimap_live.ex (1)
138-143: Field name mismatch:target_slugfield stores paths, not slugs.The
target_slugfield in the Backlink model is documented as storing "target identified bytarget_slug", but the actual data contains paths and is used as path parameters inpage_url(). The variablemissing_target_pathscorrectly reflects this semantic meaning. Update the Backlink model field totarget_pathand revise the documentation to describe paths rather than slugs, or ensure the refactoring was intentional and the field definition is genuinely outdated.
🤖 Fix all issues with AI agents
In `@assets/css/drawer2.css`:
- Around line 15-22: The .drawer2-side rule currently applies `hidden` (display:
none) which will keep the drawer invisible even when .drawer2-open toggles
transform; remove the `@apply hidden` from .drawer2-side or ensure the open
state explicitly restores display (e.g., set display/block) in the .drawer2-open
.drawer2-side rule so visibility is controlled by transform/pointer-events
rather than display; update the CSS for the selectors `.drawer2-side` and
`.drawer2-open .drawer2-side` accordingly.
In `@lib/wik_web/live/group_live/page_live/index.ex`:
- Around line 20-26: The current template builds a link using page_path
(computed by page_tree_path_for) and always calls
WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path), which
will break when page_path is nil; update the template to avoid calling page_url
when page_path is nil by conditionally rendering: if page_path is present render
the .link with navigate set to page_url(`@ctx.current_group`, page_path),
otherwise render a non-clickable element (e.g., span/div with the same classes)
or disable the link; ensure you reference page_tree_path_for and
WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path) when
changing the conditional so the navigate helper is never invoked with a nil
path.
In `@lib/wik_web/live/group_live/page_live/show/action_button.ex`:
- Around line 17-29: The button only visually indicates disabled via CSS but
still accepts clicks; update the <button> element in the component (the element
using attrs id={`@id`}, form={`@form`}, type={`@type`}, class=[...], data-tip={`@tip`},
{`@rest`}) to include the HTML disabled attribute when `@disabled` is truthy (e.g.,
disabled={`@disabled`}) and consider also adding aria-disabled={`@disabled`} for
accessibility so the button is non-interactive both functionally and
semantically.
In `@lib/wik_web/live/group_live/page_live/show/form_markdown.ex`:
- Around line 133-148: The call to Wik.Wiki.PageTree.Utils.ensure_page_for_tree
in ensure_page_tree_stubs discards its result, hiding failures; change the
reduce branch to pattern-match on ensure_page_for_tree(tree, actor) (e.g., {:ok,
page} -> return next_map; {:error, reason} -> Logger.error with context
including raw_path/tree and reason and return map to avoid advancing the
path_map on failure) while keeping the resolve_tree_by_path handling intact;
update ensure_page_tree_stubs to log errors and only use next_map when
ensure_page_for_tree succeeds.
In `@lib/wik_web/live/group_live/page_live/show/modal_usaved_exit.ex`:
- Around line 1-2: The file is misnamed: rename the file from
modal_usaved_exit.ex to modal_unsaved_exit.ex so it matches the module
WikWeb.GroupLive.PageLive.Show.ModalUnsavedExit (Elixir expects snake_case
filenames matching module names); update any references/imports/path usages if
they refer to the old filename, then run mix compile to verify there are no
remaining mismatches.
In `@lib/wik_web/live/group_live/page_live/show/page_head.ex`:
- Around line 16-54: The H1 toggle still triggers JS.toggle(to:
".pagehead-form") even when the edit form (the .form guarded by Ash.can?({`@page`,
:update}, `@current_user`)) is not rendered; update the template to conditionally
enable the toggle only when editable (e.g., add an `@editable` assign derived from
Ash.can? and use it to guard the phx-click/JS.toggle calls on the H1 and
buttons) or render a non-interactive title when not editable. Ensure the
references to JS.toggle(to: ".pagehead-form") and JS.toggle(to: ".pagehead-h1")
are only emitted when `@editable` is true so clicking the title won’t hide the H1
without showing a form.
In `@lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex`:
- Around line 10-18: The backlink rendering can produce an invalid URL when
page_tree_path_for(`@ctx`, backlink.source_page_id) returns nil; update the
rendering in the Backlinks component to either filter out such backlinks or
conditionally render them as non-clickable text: compute source_path via
page_tree_path_for for each backlink and if source_path is nil render
backlink_label(`@ctx`, backlink) inside a <li> as plain text (or add an
Enum.filter over `@backlinks` to drop items where page_tree_path_for returns nil)
otherwise render the existing .link that calls
WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, source_path).
In `@lib/wik_web/live/group_live/wikitree_live.ex`:
- Around line 126-134: The descendant check in ensure_not_descendant incorrectly
treats prefixes as descendants (e.g., "foo" vs "foobar"); change the
starts_with? test to enforce a path boundary by checking
String.starts_with?(new_parent_path, node_path <> "/") so only true descendants
(with a "/" after the node_path) are rejected, leaving the existing clause for
empty new_parent_path intact.
In `@lib/wik/notifiers/page_tree_mutation.ex`:
- Around line 6-17: The notify/1 function calls extract_group_id(notification)
and then immediately passes the result to
Wik.Wiki.PageTree.Utils.pages_tree_topic/1 which can crash if group_id is nil;
guard against that by checking group_id is not nil before deriving topic or
broadcasting (either add a when not is_nil(group_id) clause to notify/1 or an
explicit early return when extract_group_id(notification) == nil), then only
call pages_tree_topic(group_id) and Phoenix.PubSub.broadcast when group_id is
non-nil.
🧹 Nitpick comments (37)
lib/utils/boolean.ex (1)
1-8: LGTM!Clean implementation using idiomatic Elixir multi-clause dispatch. The pattern matching order is correct, and handling both string and boolean inputs is practical for form/config parsing scenarios.
Optional: Consider adding a typespec for improved documentation and dialyzer support:
`@spec` parse(String.t() | boolean() | nil | term(), term()) :: boolean() | term()lib/wik/tags/page_to_tag/sync.ex (2)
48-66: Consider prepending notifications for O(n) instead of O(n²) complexity.Using
notifications ++ new_notificationsinsideEnum.reduceis O(n²) because++traverses the left list. For consistency withtag_ids(which prepends then reverses), consider the same pattern for notifications.♻️ Suggested optimization
defp upsert_tags(group_id, tags) do {tag_ids, notifications} = Enum.reduce(tags, {[], []}, fn name, {ids, notifications} -> {:ok, tag, new_notifications} = Tag |> Ash.Changeset.for_create(:create, %{group_id: group_id, name: name}) |> Ash.create( upsert?: true, upsert_identity: :unique_group_name, upsert_fields: [:name], authorize?: false, return_notifications?: true ) - {[tag.id | ids], notifications ++ new_notifications} + {[tag.id | ids], new_notifications ++ notifications} end) - {Enum.reverse(tag_ids), notifications} + {Enum.reverse(tag_ids), Enum.reverse(notifications)} end
76-90: Same O(n²) list concatenation pattern in reduce.Similar to
upsert_tags, thenotifications ++ new_notificationsinside the reduce has quadratic complexity. Consider prepending and reversing at the return point.♻️ Suggested optimization
- Enum.reduce(tag_ids, notifications, fn tag_id, notifications -> + tag_ids + |> Enum.reduce(notifications, fn tag_id, notifications -> case PageToTag |> Ash.Changeset.for_create(:create, %{ group_id: group_id, page_id: page_id, tag_id: tag_id }) |> Ash.create(authorize?: false, return_notifications?: true) do {:ok, _page_to_tag, new_notifications} -> - notifications ++ new_notifications + new_notifications ++ notifications {:error, reason} -> raise "Failed to create PageToTag: #{inspect(reason)}" end end) + |> Enum.reverse() endNote: If notification order is not significant for your use case, you can skip the
Enum.reverse/1call entirely.assets/css/prose-markdown.css (1)
15-26: Avoid selection highlight causing layout shift.
Applying a border on selection can “jiggle” layout. Consideroutline(or a constant transparent border on all nodes) to keep dimensions stable.Proposed tweak (outline instead of border)
.ProseMirror.editor { `@apply` wrap-anywhere; .ProseMirror-selectednode { - `@apply` border border-yellow-500; + `@apply` outline outline-2 outline-yellow-500; }lib/wik_web/live/group_live/page_live/show/panels/Toc.ex (1)
4-14: Consider defensive handling foritem.leveland optional use oftruncateclass.The Tailwind classes
text-ellipsisandtext-nowrapare valid in Tailwind CSS 4.1.7 used by this project, so the current styling is appropriate. However, theitem.levelvalue used in margin-left arithmetic should have a defensive default to prevent potential crashes if the value is missing or non-numeric. Alternatively, consider using thetruncateutility class as a more concise way to handle text overflow, though the current approach is valid.Optional refactoring
def panel(assigns) do ~H""" <div :for={item <- `@toc`} - class="overflow-hidden text-ellipsis text-nowrap text-xs" - style={ "margin-left: #{( item.level - 1 )/2}rem;" } + class="truncate text-xs" + style={"margin-left: #{(max(item.level || 1, 1) - 1) / 2}rem;"} > <a class="opacity-70 hover:opacity-100 transition" href={ "##{item.slug}" }> {item.title} </a> </div> """ endassets/js/hooks/milkdown/slash-menus/slash-menu-wikilinks.ts (1)
96-104: Consider whether fuzzy matching should combine scores from both label and path.The current logic
fuzzyScore(p.label, q) || fuzzyScore(p.path, q)short-circuits: if the label matches, the path score is ignored. This means a page with a weak label match but a strong path match will rank based only on the label score.If you want to consider both for ranking, you might use addition instead:
- score: fuzzyScore(p.label, q) || fuzzyScore(p.path, q), + score: fuzzyScore(p.label, q) + fuzzyScore(p.path, q),However, if prioritizing label matches over path matches is intentional, the current approach is fine.
priv/repo/BAK/20260103200000_add_pages_tree.exs (1)
1-58: Clarify the purpose of this backup migration file.This migration is in a
BAKfolder, while a similar migration exists atpriv/repo/migrations/20260104002532_migrate_resources6_dev.exs. Having two migrations creating the same table could cause confusion or conflicts.Consider:
- Removing this backup file if it's superseded by the migrations folder version
- Adding a comment explaining why both exist if this is intentional
- Moving it to a clearly named archive directory
priv/repo/migrations/20260104002532_migrate_resources6_dev.exs (1)
44-46: Consider adding indexes on foreign key columns for query performance.Foreign key columns
group_idandpage_idare frequently used in lookups and joins. While the unique index coversgroup_idin combination withpath, standalone queries filtering bygroup_idalone or bypage_idwould benefit from dedicated indexes.Suggested addition
create unique_index(:pages_tree, [:group_id, :path], name: "pages_tree_unique_group_path_index" ) + + create index(:pages_tree, [:page_id]) + create index(:pages_tree, [:group_id]) endlib/wik_web/live/group_live/page_live/show/action_button.ex (1)
12-12: Remove unusedvariantattribute or implement its styling.The
variantattribute is declared with values[:neutral, :accent, :primary]but is never used in the render function. Either remove it to avoid confusion, or implement the variant-based styling.Option 1: Remove unused attribute
- attr :variant, :atom, values: [:neutral, :accent, :primary], default: :neutralOption 2: Implement variant styling
class={[ "aspect-square w-10", if(`@disabled`, do: "opacity-20", else: "cursor-pointer"), if(`@hidden`, do: "hidden"), + case `@variant` do + :accent -> "text-accent" + :primary -> "text-primary" + _ -> "text-neutral" + end, `@tip` && "tooltip", `@class` ]}lib/wik_web/live/group_live/page_live/show/actions.ex (1)
45-68: Consider addingidattributes to all ActionButton components for consistency.Some
ActionButton.rendercalls (Lines 45, 56, 64) lack anidprop while others include it. Adding unique IDs to all buttons improves debuggability and allows targeting specific buttons in tests or JS hooks.Suggested addition of IDs
<ActionButton.render :if={Ash.can?({`@page`, :update}, `@current_user`)} + id={"editor-edit-#{`@page.id`}"} class={["opacity-50 hover:opacity-100 transition"]} phx-click="toggle_editing" tip="Edit page" icon="hero-pencil-solid" /><ActionButton.render + id={"editor-toggle-source-#{`@page.id`}"} phx-click="toggle_source" class={[if(!@source?, do: "opacity-50 hover:opacity-100 transition", else: "bg-primary")]} tip={if(`@source`?, do: "Hide source", else: "Show source")} icon="hero-hashtag-micro" /><ActionButton.render + id={"editor-toggle-open-#{`@page.id`}"} phx-click="toggle_open?" class={[`@open`? && "hidden", "md:hidden", "opacity-50 hover:opacity-100 transition"]} icon="hero-chevron-left-micro" />lib/wik_web/live/tag_live/show.ex (1)
225-230: Duplicated helper function.This
page_tree_path_for/2helper is identical to the one inlib/wik_web/live/group_live/page_live/index.ex(Lines 113-118). Consider extracting it to a shared module (e.g.,WikWeb.Helpers.PageTree) to reduce duplication.lib/wik_web/live/group_live/page_live/show/utils.ex (3)
3-3: Consider removinguse WikWeb, :live_viewfrom this utility module.This module is a utility helper, not a LiveView. The
use WikWeb, :live_viewmacro typically imports LiveView-specific functionality that isn't needed here. The module only needsrequire Ash.Queryfor query building.
94-156: Duplicatedmove_subtree/4implementation.This function is nearly identical to the one in
lib/wik_web/live/group_live/wikitree_live.ex(lines 146-210). The only difference is that the wikitree_live.ex version includesLogger.errorcalls for failure cases, while this version does not.Consider extracting this into a shared module (perhaps
Wik.Wiki.PageTree.Utils) to avoid duplication and ensure consistent error logging.💡 Suggested approach
Move
move_subtree/4toWik.Wiki.PageTree.Utilsand call it from both locations:- defp move_subtree(group_id, actor, old_path, new_path) do - # ... 60+ lines of duplicated code - end + # In this module: + defp move_subtree(group_id, actor, old_path, new_path) do + PageTreeUtils.move_subtree(group_id, actor, old_path, new_path) + endAlso add error logging to match the wikitree_live.ex behavior.
145-146: Missing error logging for failure cases.Unlike the similar implementation in
wikitree_live.ex, this version silently returns error tuples without logging. This could make debugging production issues difficult.📝 Proposed fix
{:error, _} -> - {:error, :update_failed} + Logger.error("Failed to move pages_tree subtree during rename") + {:error, :update_failed} end else {:error, :conflict} end end - {:error, _} -> - {:error, :load_failed} + {:error, error} -> + Logger.error("Failed to load pages_tree: #{inspect(error)}") + {:error, :load_failed}Also add
require Loggerat the top of the module.Also applies to: 153-154
lib/wik/wiki/page_tree/utils.ex (2)
60-83: Consider whether read errors should create new trees.On line 78-79, when
Ash.read_onereturns{:error, _}, the code falls through tocreate_tree/5. This masks the original error and attempts creation even when there might be a database connectivity issue or authorization problem.📝 Proposed fix
{:ok, nil} -> create_tree(normalized, title, group_id, actor, path_map) - {:error, _} -> - create_tree(normalized, title, group_id, actor, path_map) + {:error, reason} -> + {:error, "Failed to query page tree: #{Exception.message(reason)}"} end
146-153: Themap_entry/2helper returns inconsistent structures.The function returns different shapes depending on input:
%{id: _, ...}→ returns as-isid(binary) → returns%{id: id, path: normalized}(synthetic struct)- Other → returns as-is
This inconsistency could lead to issues if callers expect a
PageTreestruct but receive the synthetic map from line 150.lib/wik_web/ctx_additions.ex (1)
54-60: Struct interpolation may produce unreadable output.The error log interpolates
current_groupandcurrent_userdirectly. WhileGrouphas aString.Charsimplementation (returnsgroup.title),Usermay not, which could result in output like#Wik.Accounts.User<...>or a protocol error.📝 Proposed fix
Logger.error(""" 🔴 Failed to read page tree for: - Group: #{current_group} | id: #{current_group.id} - User: #{current_user} | id: #{current_user.id} + Group: #{current_group.id} + User: #{current_user && current_user.id} Error: #{Exception.format(:error, error)} """)lib/wik/accounts/group.ex (1)
231-251: Consider extracting the tree-by-page-id loading pattern.This pattern of loading PageTree entries and reducing them to a map by
page_idappears in at least three places:
- Here in
load_tree_by_page_id/2lib/wik_web/ctx_additions.ex(lines 39-48)lib/wik_web/live/group_live/page_live/show/utils.ex(lines 32-41)Consider extracting this to
Wik.Wiki.PageTree.Utilsto reduce duplication.lib/wik_web/live/group_live/page_live/show/panels/Debug.ex (1)
6-14: Minor HTML semantic inconsistency.Line 7 uses
<dd>without a parent<dl>element, while the remaining items use<div>. Consider using<div>consistently for this debug panel, or wrap all items in a proper<dl>/<dt>/<dd>structure.💡 Option: Use consistent `
` tags<div class="font-mono text-xs opacity-70"> - <dd>page path: {`@page_tree_path`}</dd> + <div>page path: {`@page_tree_path`}</div> <div>editing?: {`@editing`?}</div> <div>synced?: {`@editor_state.synced`?}</div> <div>has_undo?: {`@editor_state.has_undo`?}</div> <div>has_redo?: {`@editor_state.has_redo`?}</div> <div>exit_after_save?: {`@exit_after_save`?}</div> <div>show_unsaved_modal?: {`@show_unsaved_modal`}</div> </div>test/wik/wiki/backlink/util_test.exs (1)
59-82: Consider extracting the repeated PageTree resolution pattern into a helper.Both tests (lines 38-42 and 65-68) use an identical pattern for resolving and ensuring the tree. This is a minor duplication.
♻️ Optional refactor to reduce duplication
defp resolve_and_ensure_tree(page, group, user) do {:ok, tree, _map} = PageTree.Utils.resolve_tree_by_path(page.slug, group.id, user, %{}) {:ok, ensured_tree} = PageTree.Utils.ensure_page_for_tree(tree, user) ensured_tree endThen in tests:
- {:ok, tree, _map} = - PageTree.Utils.resolve_tree_by_path(target.slug, group.id, user, %{}) - - {:ok, ensured_tree} = PageTree.Utils.ensure_page_for_tree(tree, user) + ensured_tree = resolve_and_ensure_tree(target, group, user)assets/js/hooks/milkdown/input-rule-wikilink.ts (1)
7-7: Consider removing the global flaggfrom the regex.The InputRule callback processes matches one at a time. The global flag
gcan cause issues with regexlastIndexstate across invocations. Standard InputRule patterns typically omit this flag.♻️ Suggested fix
- new InputRule(/\[\[([^\]]+)\]\]/g, (state, match, start, end) => { + new InputRule(/\[\[([^\]]+)\]\]/, (state, match, start, end) => {lib/wik_web/live/group_live/page_live/show/panels.ex (1)
41-46: Consider removing commented-out code or adding a TODO comment.The commented Descendants panel block is a placeholder. If this is planned work, consider adding a
# TODO:comment explaining what's pending. If it's obsolete, remove it to keep the codebase clean.assets/js/hooks/wikitree.js (1)
62-66: Page reload on move failure is a blunt recovery mechanism.While the reload ensures consistency after a failed move, it results in a poor user experience. Consider showing a toast/flash message and reverting the tree to its previous state instead.
lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex (1)
24-29: Consider makingpage_tree_path_forprivate.This function is declared as
def(public), but it appears to be an internal helper specific to this module's rendering logic. If it's not intended to be called from outside this module, consider usingdefpfor encapsulation.- def page_tree_path_for(ctx, page_id) do + defp page_tree_path_for(ctx, page_id) dolib/wik_web/live/group_live/page_live/show/breadcrumbs.ex (2)
25-40: Unused index variable in comprehension.The
_idxfromEnum.with_indexis never used. You can simplify by removing it.- <%= for {crumb, _idx} <- Enum.with_index(`@breadcrumbs`) do %> + <%= for crumb <- `@breadcrumbs` do %>
4-5: Remove redundantuse Phoenix.Component.
use WikWeb, :htmlalready includesuse Phoenix.Component(defined inWikWeb.htmlat line 69 of lib/wik_web.ex). Line 5 can be safely removed.lib/wik_web/live/group_live/page_live/index.ex (1)
114-119: Duplicate helper function across modules.This
page_tree_path_for/2implementation is identical to the one inlib/wik_web/live/tag_live/show.ex(lines 224-229). Consider extracting this to a shared utility module to avoid duplication.Suggested approach
Extract to a shared module like
WikWeb.PageTreeHelpers:defmodule WikWeb.PageTreeHelpers do def path_for(ctx, page_id) do case Map.get(ctx.pages_tree_by_page_id || %{}, page_id) do %{path: path} when is_binary(path) and path != "" -> path _ -> nil end end endThen import or alias in both LiveViews.
lib/wik_web/live/group_live/page_live/show/page_head.ex (1)
6-11: Consider accessibility for the clickable heading.The H1 element is clickable but lacks keyboard accessibility. Screen reader users and keyboard-only users cannot activate this toggle.
Suggested fix to add keyboard support
<h1 class="pagehead-h1 text-3xl mb-8 hover:bg-base-300/20 cursor-pointer" + tabindex="0" + role="button" + phx-keydown={JS.toggle() |> JS.toggle(to: ".pagehead-form")} + phx-key="Enter" phx-click={JS.toggle() |> JS.toggle(to: ".pagehead-form")} > {`@input`} </h1>assets/css/tui-tree.css (3)
36-36: Duplicatetext-ellipsisclass.The class
text-ellipsisis applied twice on the same line.Fix
- `@apply` w-64 overflow-hidden text-ellipsis text-ellipsis text-nowrap; + `@apply` w-64 overflow-hidden text-ellipsis text-nowrap;
69-72: Redundant styling - Tailwind@applyoverwritten by raw CSS.Line 70 uses
@apply bg-[rgba(75,150,230,0.1)]but line 71 immediately overwrites it withbackground:#e7eff7``. The@applydirective is ineffective.Suggested fix
Either remove the
@applyor use only Tailwind:.tui-tree-selected > .tui-tree-content-wrapper { - `@apply` bg-[rgba(75,150,230,0.1)]; - background: `#e7eff7`; + `@apply` bg-[`#e7eff7`]; }
87-131: Large block of commented-out code.Consider removing commented-out CSS if it's no longer needed, or add a brief comment explaining why it's kept for reference.
lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (2)
4-14: Redundant tree computation.
build_descendant_tree/2is called twice: once on line 5 (result assigned todescendants) and again on line 10 in the template. The assignment on lines 5-6 is unused. Use the already-computed@descendantsassign instead.♻️ Proposed fix
def panel(assigns) do descendants = build_descendant_tree(assigns.page_tree_path, assigns.ctx.pages_tree_map) assigns = assigns |> assign(:descendants, descendants) ~H""" <.descendants_list - nodes={build_descendant_tree(`@page_tree_path`, `@ctx.pages_tree_map`)} + nodes={`@descendants`} ctx={`@ctx`} /> """ end
73-94: Default parameter on multi-clause function.In Elixir, default parameters on functions with multiple clauses require a separate function head. While this works because there's only one clause for
insert_descendant_node/5, the defaultpath \\ []would cause issues if you add pattern-matching clauses later. Consider using a separate function head or a helper wrapper for clarity.♻️ Suggested pattern
+def insert_descendant_node(nodes, segments, current_path, pages_tree_map) do + insert_descendant_node(nodes, segments, current_path, pages_tree_map, []) +end + -def insert_descendant_node(nodes, [segment | rest], current_path, pages_tree_map, path \\ []) do +defp insert_descendant_node(nodes, [segment | rest], current_path, pages_tree_map, path) do # ... rest unchanged endlib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
5-26: Unused assigns:descendantsandtree_visible?.The
descendants(lines 6-10) andtree_visible?(line 21) are computed and assigned but never referenced in the template. If these are for future use, consider removing them to avoid unnecessary computation, or add a TODO comment indicating planned usage.♻️ Proposed cleanup
def panel(assigns) do - descendants = - Descendants.build_descendant_tree( - assigns.page_tree_path, - assigns.ctx.pages_tree_map - ) - tree_include_siblings? = true tree_nodes = build_tree(assigns.page_tree_path, assigns.ctx.pages_tree_map, tree_include_siblings?) assigns = assigns - |> assign(:descendants, descendants) |> assign(:tree_nodes, tree_nodes) - |> assign(:tree_visible?, tree_visible?(assigns.page_tree_path, assigns.ctx.pages_tree_map)) ~H""" <.tree_list nodes={`@tree_nodes`} ctx={`@ctx`} current_path={`@page_tree_path`} /> """ endlib/wik_web/live/group_live/page_live/history.ex (1)
145-174: Consider more specific error handling in mount.The catch-all
_ ->clause on line 167 swallows all errors from thewithchain. While this ensures the page always renders (showing "not found"), it may hide unexpected failures during development. Consider logging the error or matching specific failure patterns.♻️ Suggested improvement
else - _ -> + {:error, reason} -> + require Logger + Logger.warning("Failed to resolve page tree for path #{inspect(page_path)}: #{inspect(reason)}") {:ok, socket |> assign(:not_found?, true) |> assign(:page, nil) |> assign(:page_tree_path, page_path) |> assign(:page_title, "Page not found")}lib/wik_web/live/group_live/wikitree_live.ex (1)
220-227: Redundant empty segment filtering.
String.split("/", trim: true)already removes empty strings from the result, making theEnum.reject(&(&1 == ""))call unnecessary.♻️ Suggested simplification
defp insert_tree_path(nodes, path) do - segments = - path - |> String.split("/", trim: true) - |> Enum.reject(&(&1 == "")) - - insert_segments(nodes, segments) + path + |> String.split("/", trim: true) + |> then(&insert_segments(nodes, &1)) endlib/wik_web/components/layouts.ex (1)
75-80: Consider adding keyboard accessibility to the backdrop.The backdrop
divwithphx-click="toggle_open?"is clickable but lacks keyboard accessibility attributes. Screen reader users and keyboard-only users cannot interact with it.♿ Suggested accessibility improvement
<div :if={`@open`?} phx-click="toggle_open?" + phx-key="Escape" + phx-window-keydown="toggle_open?" + role="button" + tabindex="0" + aria-label="Close sidebar" class="bg-base-300/30 hover:bg-base-300/20 transition relative md:hidden cursor-pointer" />Alternatively, consider using a
<button>element for the backdrop overlay.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
assets/package-lock.jsonis excluded by!**/package-lock.jsonmix.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
assets/css/app.cssassets/css/drawer2.cssassets/css/prose-markdown.cssassets/css/tui-tree.cssassets/js/app.jsassets/js/hooks/milkdown.tsassets/js/hooks/milkdown/collab.tsassets/js/hooks/milkdown/input-rule-wikilink.tsassets/js/hooks/milkdown/setup.tsassets/js/hooks/milkdown/slash-menus/slash-menu-wikilinks.tsassets/js/hooks/milkdown/toolbar/command-toggle-wiki-link.tsassets/js/hooks/milkdown/wikilink-node.tsassets/js/hooks/milkdown/wikilink-plugin.tsassets/js/hooks/wikimap.jsassets/js/hooks/wikitree.jsassets/package.jsonlib/utils/boolean.exlib/wik/accounts/group.exlib/wik/notifiers/page_tree_mutation.exlib/wik/tags/page_to_tag/sync.exlib/wik/wiki.exlib/wik/wiki/backlink/utils.exlib/wik/wiki/page.exlib/wik/wiki/page_tree.exlib/wik/wiki/page_tree/markdown.exlib/wik/wiki/page_tree/utils.exlib/wik_web/components/components.exlib/wik_web/components/layouts.exlib/wik_web/components/page/breadcrumbs.exlib/wik_web/components/page/form.exlib/wik_web/ctx_additions.exlib/wik_web/live/group_live/member_live/index.exlib/wik_web/live/group_live/page_live/history.exlib/wik_web/live/group_live/page_live/index.exlib/wik_web/live/group_live/page_live/show.exlib/wik_web/live/group_live/page_live/show/action_button.exlib/wik_web/live/group_live/page_live/show/actions.exlib/wik_web/live/group_live/page_live/show/breadcrumbs.exlib/wik_web/live/group_live/page_live/show/form_markdown.exlib/wik_web/live/group_live/page_live/show/modal_usaved_exit.exlib/wik_web/live/group_live/page_live/show/page_head.exlib/wik_web/live/group_live/page_live/show/panels.exlib/wik_web/live/group_live/page_live/show/panels/Backlinks.exlib/wik_web/live/group_live/page_live/show/panels/Debug.exlib/wik_web/live/group_live/page_live/show/panels/Descendants.exlib/wik_web/live/group_live/page_live/show/panels/Toc.exlib/wik_web/live/group_live/page_live/show/panels/Tree.exlib/wik_web/live/group_live/page_live/show/panels/Versions.exlib/wik_web/live/group_live/page_live/show/utils.exlib/wik_web/live/group_live/show.exlib/wik_web/live/group_live/wikimap_live.exlib/wik_web/live/group_live/wikitree_live.exlib/wik_web/live/home_live.exlib/wik_web/live/tag_live/index.exlib/wik_web/live/tag_live/show.exlib/wik_web/router.exmix.exspriv/repo/BAK/20260103200000_add_pages_tree.exspriv/repo/migrations/20260104002532_migrate_resources6_dev.exspriv/resource_snapshots/repo/pages_tree/20260104002532_dev.jsontest/wik/wiki/backlink/cascade_test.exstest/wik/wiki/backlink/util_test.exs
💤 Files with no reviewable changes (2)
- lib/wik_web/components/page/breadcrumbs.ex
- lib/wik_web/components/page/form.ex
🧰 Additional context used
🧬 Code graph analysis (19)
assets/js/hooks/milkdown/slash-menus/slash-menu-wikilinks.ts (1)
assets/js/hooks/milkdown/slash-menus/components/slash-menu-view.ts (1)
SlashMenuItem(5-5)
lib/wik_web/live/group_live/page_live/show/panels/Debug.ex (6)
lib/wik_web/components/layouts.ex (1)
panel(34-45)lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex (1)
panel(4-22)lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (1)
panel(4-14)lib/wik_web/live/group_live/page_live/show/panels/Toc.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
panel(5-26)lib/wik_web/live/group_live/page_live/show/panels/Versions.ex (1)
panel(4-31)
lib/wik_web/live/group_live/page_live/show/breadcrumbs.ex (1)
lib/wik_web/live/group_live/page_live/show.ex (1)
render(24-69)
lib/wik_web/live/group_live/page_live/index.ex (1)
lib/wik_web/live/tag_live/show.ex (1)
page_tree_path_for(225-230)
lib/wik_web/live/group_live/page_live/show/action_button.ex (1)
lib/wik_web/live/group_live/page_live/show/actions.ex (1)
render(5-71)
lib/wik_web/live/group_live/page_live/show/panels/Versions.ex (6)
lib/wik_web/components/layouts.ex (1)
panel(34-45)lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex (1)
panel(4-22)lib/wik_web/live/group_live/page_live/show/panels/Debug.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (1)
panel(4-14)lib/wik_web/live/group_live/page_live/show/panels/Toc.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
panel(5-26)
lib/wik_web/live/group_live/page_live/show/modal_usaved_exit.ex (1)
lib/wik_web/live/group_live/page_live/show.ex (1)
render(24-69)
lib/wik/wiki/backlink/utils.ex (1)
lib/wik/wiki/page_tree/utils.ex (1)
ensure_page_for_tree(91-127)
assets/js/hooks/milkdown/input-rule-wikilink.ts (1)
assets/js/hooks/milkdown/undo-state.test.js (1)
schema(16-22)
lib/wik_web/live/group_live/page_live/show/panels.ex (2)
lib/wik_web/live/group_live/page_live/show.ex (1)
render(24-69)lib/wik_web/live/group_live/page_live/show/breadcrumbs.ex (1)
render(12-45)
lib/wik_web/ctx_additions.ex (2)
lib/utils/ctx.ex (1)
add(4-8)lib/wik_web/components/core_components.ex (1)
error(275-282)
lib/wik_web/live/group_live/page_live/show/form_markdown.ex (2)
lib/wik/wiki/page_tree/markdown.ex (2)
extract_wikilinks(26-34)rewrite_wikilinks(37-61)lib/wik/wiki/page_tree/utils.ex (2)
resolve_tree_by_path(62-83)ensure_page_for_tree(91-127)
assets/js/hooks/milkdown/setup.ts (2)
assets/js/hooks/milkdown/wikilink-node.ts (1)
configureWikilinkMarkdown(75-91)assets/js/hooks/milkdown/sanitize-doc.ts (1)
sanitizeDocPlugin(11-24)
lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (1)
lib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
panel(5-26)
lib/wik_web/live/group_live/page_live/show/utils.ex (3)
lib/wik_web/live/group_live/wikitree_live.ex (1)
move_subtree(147-211)lib/utils/ctx.ex (1)
add(4-8)lib/wik_web/live/group_live/page_live/show/form_markdown.ex (1)
update(72-75)
lib/wik_web/live/tag_live/show.ex (2)
lib/wik/accounts/group.ex (1)
load(191-193)lib/wik_web/live/group_live/page_live/index.ex (1)
page_tree_path_for(114-119)
lib/wik_web/live/group_live/page_live/show/actions.ex (1)
lib/wik_web/live/group_live/page_live/show/action_button.ex (1)
render(15-34)
lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex (6)
lib/wik_web/components/layouts.ex (1)
panel(34-45)lib/wik_web/live/group_live/page_live/show/panels/Debug.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (1)
panel(4-14)lib/wik_web/live/group_live/page_live/show/panels/Toc.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
panel(5-26)lib/wik_web/live/group_live/page_live/show/panels/Versions.ex (1)
panel(4-31)
lib/wik_web/components/layouts.ex (6)
lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex (1)
panel(4-22)lib/wik_web/live/group_live/page_live/show/panels/Debug.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Descendants.ex (1)
panel(4-14)lib/wik_web/live/group_live/page_live/show/panels/Toc.ex (1)
panel(4-16)lib/wik_web/live/group_live/page_live/show/panels/Tree.ex (1)
panel(5-26)lib/wik_web/live/group_live/page_live/show/panels/Versions.ex (1)
panel(4-31)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| .drawer2-side { | ||
| @apply hidden; | ||
| @apply start-auto end-0 fixed inset-y-0; | ||
| > :first-child { | ||
| @apply h-full w-80 max-w-[80vw]; | ||
| @apply will-change-transform; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential CSS conflict: hidden may prevent drawer from appearing.
The @apply hidden on line 16 sets display: none on .drawer2-side. However, the .drawer2-open .drawer2-side rule (lines 33-38) only changes pointer-events and transform, without overriding display. This means the drawer panel may never become visible when opened.
Consider removing hidden and relying solely on the transform-based off-screen positioning, or explicitly set display: block (or similar) in the .drawer2-open .drawer2-side rule.
Proposed fix
.drawer2-side {
- `@apply` hidden;
+ `@apply` block;
`@apply` start-auto end-0 fixed inset-y-0;
> :first-child {
`@apply` h-full w-80 max-w-[80vw];📝 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.
| .drawer2-side { | |
| @apply hidden; | |
| @apply start-auto end-0 fixed inset-y-0; | |
| > :first-child { | |
| @apply h-full w-80 max-w-[80vw]; | |
| @apply will-change-transform; | |
| } | |
| } | |
| .drawer2-side { | |
| `@apply` block; | |
| `@apply` start-auto end-0 fixed inset-y-0; | |
| > :first-child { | |
| `@apply` h-full w-80 max-w-[80vw]; | |
| `@apply` will-change-transform; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@assets/css/drawer2.css` around lines 15 - 22, The .drawer2-side rule
currently applies `hidden` (display: none) which will keep the drawer invisible
even when .drawer2-open toggles transform; remove the `@apply hidden` from
.drawer2-side or ensure the open state explicitly restores display (e.g., set
display/block) in the .drawer2-open .drawer2-side rule so visibility is
controlled by transform/pointer-events rather than display; update the CSS for
the selectors `.drawer2-side` and `.drawer2-open .drawer2-side` accordingly.
| buildTree(raw) { | ||
| let data = []; | ||
| try { | ||
| data = JSON.parse(raw); | ||
| } catch (error) { | ||
| console.error("Invalid wikitree data", error); | ||
| return; | ||
| } | ||
|
|
||
| this.el.dataset.graph = raw; | ||
| this.el.innerHTML = ""; | ||
| this.tree = new Tree(this.el, { | ||
| data, | ||
| nodeDefaultState: "opened", | ||
| usageStatistics: false, | ||
| }).enableFeature("Draggable", { | ||
| helperClassName: "tui-tree-drop", | ||
| lineClassName: "tui-tree-line", | ||
| isSortable: true, | ||
| }); |
There was a problem hiding this comment.
Memory leak risk: previous tree instance not cleaned up before rebuild.
When buildTree is called via wikitree_refresh, the previous this.tree instance isn't destroyed before creating a new one. This may cause memory leaks or event handler duplication if the tui-tree library doesn't handle this internally.
Proposed fix
buildTree(raw) {
+ // Clean up previous tree instance if it exists
+ if (this.tree) {
+ if (typeof this.tree.disableFeature === "function") {
+ this.tree.disableFeature("Draggable");
+ }
+ this.tree = null;
+ }
+
let data = [];
try {
data = JSON.parse(raw);📝 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.
| buildTree(raw) { | |
| let data = []; | |
| try { | |
| data = JSON.parse(raw); | |
| } catch (error) { | |
| console.error("Invalid wikitree data", error); | |
| return; | |
| } | |
| this.el.dataset.graph = raw; | |
| this.el.innerHTML = ""; | |
| this.tree = new Tree(this.el, { | |
| data, | |
| nodeDefaultState: "opened", | |
| usageStatistics: false, | |
| }).enableFeature("Draggable", { | |
| helperClassName: "tui-tree-drop", | |
| lineClassName: "tui-tree-line", | |
| isSortable: true, | |
| }); | |
| buildTree(raw) { | |
| // Clean up previous tree instance if it exists | |
| if (this.tree) { | |
| if (typeof this.tree.disableFeature === "function") { | |
| this.tree.disableFeature("Draggable"); | |
| } | |
| this.tree = null; | |
| } | |
| let data = []; | |
| try { | |
| data = JSON.parse(raw); | |
| } catch (error) { | |
| console.error("Invalid wikitree data", error); | |
| return; | |
| } | |
| this.el.dataset.graph = raw; | |
| this.el.innerHTML = ""; | |
| this.tree = new Tree(this.el, { | |
| data, | |
| nodeDefaultState: "opened", | |
| usageStatistics: false, | |
| }).enableFeature("Draggable", { | |
| helperClassName: "tui-tree-drop", | |
| lineClassName: "tui-tree-line", | |
| isSortable: true, | |
| }); |
| <% page_path = page_tree_path_for(@ctx, page.id) %> | ||
| <div class="grid grid-cols-[4fr_2fr_1fr_1fr_1fr_auto] gap-x-8 items-baseline"> | ||
| <.link | ||
| navigate={WikWeb.GroupLive.PageLive.Show.page_url(@ctx.current_group, page)} | ||
| navigate={WikWeb.GroupLive.PageLive.Show.page_url(@ctx.current_group, page_path)} | ||
| class="title hover:text-white text-sm text-balance break-all" | ||
| > | ||
| {page.title} | ||
| {page_path || "Unknown"} |
There was a problem hiding this comment.
Navigation with nil path may cause issues.
When page_path is nil, the navigation URL will be malformed since page_url/2 likely expects a valid path string. While "Unknown" is displayed as a fallback label, clicking the link would navigate to an invalid URL.
Consider either disabling the link when page_path is nil or filtering out pages without a valid path from the stream.
Suggested fix to disable link when path is nil
<% page_path = page_tree_path_for(`@ctx`, page.id) %>
<div class="grid grid-cols-[4fr_2fr_1fr_1fr_1fr_auto] gap-x-8 items-baseline">
- <.link
- navigate={WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path)}
- class="title hover:text-white text-sm text-balance break-all"
- >
- {page_path || "Unknown"}
- </.link>
+ <%= if page_path do %>
+ <.link
+ navigate={WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path)}
+ class="title hover:text-white text-sm text-balance break-all"
+ >
+ {page_path}
+ </.link>
+ <% else %>
+ <span class="title text-sm text-balance break-all opacity-50">Unknown</span>
+ <% end %>🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/page_live/index.ex` around lines 20 - 26, The
current template builds a link using page_path (computed by page_tree_path_for)
and always calls WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`,
page_path), which will break when page_path is nil; update the template to avoid
calling page_url when page_path is nil by conditionally rendering: if page_path
is present render the .link with navigate set to page_url(`@ctx.current_group`,
page_path), otherwise render a non-clickable element (e.g., span/div with the
same classes) or disable the link; ensure you reference page_tree_path_for and
WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, page_path) when
changing the conditional so the navigate helper is never invoked with a nil
path.
| @impl true | ||
| def mount(%{"page_slug_segments" => page_slug_segments}, _session, socket) do | ||
| page_slug = page_slug_segments |> Enum.join("/") | ||
| page_path = page_slug_segments |> Enum.join("/") | ||
| current_group = socket.assigns.ctx.current_group | ||
| current_user = socket.assigns.current_user | ||
|
|
||
| case Wik.Wiki.Page | ||
| |> Ash.get( | ||
| %{group_id: current_group.id, slug: page_slug}, | ||
| actor: current_user, | ||
| load: [:versions_count] | ||
| ) do | ||
| {:ok, page} -> | ||
| if connected?(socket) do | ||
| Phoenix.PubSub.subscribe(Wik.PubSub, "page:updated:#{page.id}") | ||
| Phoenix.PubSub.subscribe(Wik.PubSub, "page:destroyed:#{page.id}") | ||
| end | ||
|
|
||
| socket = socket |> Utils.Ctx.add(:page, page) | ||
|
|
||
| {:ok, | ||
| socket | ||
| |> assign(:env, @env) | ||
| |> assign(:debug?, false) | ||
| |> assign(:not_found?, false) | ||
| |> assign(:page_title, page.title) | ||
| |> set_editing(false) | ||
| |> assign(:source?, false) | ||
| |> assign(:editor_state, %{synced?: true, has_undo?: false, has_redo?: false}) | ||
| |> assign(:show_unsaved_modal, false) | ||
| |> assign(:exit_after_save?, false) | ||
| |> assign(:page, page) | ||
| |> assign(:updated_fields, []) | ||
| |> assign(:backlinks, load_backlinks(page)) | ||
| |> assign(:toc, Utils.Markdown.extract_toc(page.text)) | ||
| |> maybe_subscribe_backlinks(page)} | ||
|
|
||
| {:error, _error} -> | ||
| {:ok, | ||
| socket | ||
| |> assign(:env, @env) | ||
| |> assign(:debug?, false) | ||
| |> assign(:not_found?, true) | ||
| |> assign(:page_title, "Page not found") | ||
| |> set_editing(false) | ||
| |> assign(:source?, false) | ||
| |> assign(:editor_state, %{synced?: true, has_undo?: false, has_redo?: false}) | ||
| |> assign(:show_unsaved_modal, false) | ||
| |> assign(:exit_after_save?, false) | ||
| |> assign(:updated_fields, []) | ||
| |> assign(:backlinks, [])} | ||
| pages_tree_map = socket.assigns.ctx.pages_tree_map || %{} | ||
|
|
||
| with {:ok, page, ensured_tree, updated_map} <- | ||
| load_page_from_path(page_path, current_group, current_user, pages_tree_map) do | ||
| {:ok, | ||
| socket | ||
| |> maybe_subscribe_page(current_group, page) | ||
| |> maybe_subscribe_backlinks(page) | ||
| |> assign_loaded_page(page, ensured_tree, updated_map)} | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing error handling in mount.
The with block only handles the success case. If load_page_from_path returns an error (e.g., page not found, database error), the mount will crash because there's no else clause to handle error tuples.
🐛 Proposed fix
def mount(%{"page_slug_segments" => page_slug_segments}, _session, socket) do
page_path = page_slug_segments |> Enum.join("/")
current_group = socket.assigns.ctx.current_group
current_user = socket.assigns.current_user
pages_tree_map = socket.assigns.ctx.pages_tree_map || %{}
- with {:ok, page, ensured_tree, updated_map} <-
- load_page_from_path(page_path, current_group, current_user, pages_tree_map) do
- {:ok,
- socket
- |> maybe_subscribe_page(current_group, page)
- |> maybe_subscribe_backlinks(page)
- |> assign_loaded_page(page, ensured_tree, updated_map)}
+ case load_page_from_path(page_path, current_group, current_user, pages_tree_map) do
+ {:ok, page, ensured_tree, updated_map} ->
+ {:ok,
+ socket
+ |> maybe_subscribe_page(current_group, page)
+ |> maybe_subscribe_backlinks(page)
+ |> assign_loaded_page(page, ensured_tree, updated_map)}
+
+ {:error, _reason} ->
+ {:ok,
+ socket
+ |> put_flash(:error, "Page could not be loaded")
+ |> push_navigate(to: ~p"/#{current_group.slug}/wiki")}
end
end📝 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.
| @impl true | |
| def mount(%{"page_slug_segments" => page_slug_segments}, _session, socket) do | |
| page_slug = page_slug_segments |> Enum.join("/") | |
| page_path = page_slug_segments |> Enum.join("/") | |
| current_group = socket.assigns.ctx.current_group | |
| current_user = socket.assigns.current_user | |
| case Wik.Wiki.Page | |
| |> Ash.get( | |
| %{group_id: current_group.id, slug: page_slug}, | |
| actor: current_user, | |
| load: [:versions_count] | |
| ) do | |
| {:ok, page} -> | |
| if connected?(socket) do | |
| Phoenix.PubSub.subscribe(Wik.PubSub, "page:updated:#{page.id}") | |
| Phoenix.PubSub.subscribe(Wik.PubSub, "page:destroyed:#{page.id}") | |
| end | |
| socket = socket |> Utils.Ctx.add(:page, page) | |
| {:ok, | |
| socket | |
| |> assign(:env, @env) | |
| |> assign(:debug?, false) | |
| |> assign(:not_found?, false) | |
| |> assign(:page_title, page.title) | |
| |> set_editing(false) | |
| |> assign(:source?, false) | |
| |> assign(:editor_state, %{synced?: true, has_undo?: false, has_redo?: false}) | |
| |> assign(:show_unsaved_modal, false) | |
| |> assign(:exit_after_save?, false) | |
| |> assign(:page, page) | |
| |> assign(:updated_fields, []) | |
| |> assign(:backlinks, load_backlinks(page)) | |
| |> assign(:toc, Utils.Markdown.extract_toc(page.text)) | |
| |> maybe_subscribe_backlinks(page)} | |
| {:error, _error} -> | |
| {:ok, | |
| socket | |
| |> assign(:env, @env) | |
| |> assign(:debug?, false) | |
| |> assign(:not_found?, true) | |
| |> assign(:page_title, "Page not found") | |
| |> set_editing(false) | |
| |> assign(:source?, false) | |
| |> assign(:editor_state, %{synced?: true, has_undo?: false, has_redo?: false}) | |
| |> assign(:show_unsaved_modal, false) | |
| |> assign(:exit_after_save?, false) | |
| |> assign(:updated_fields, []) | |
| |> assign(:backlinks, [])} | |
| pages_tree_map = socket.assigns.ctx.pages_tree_map || %{} | |
| with {:ok, page, ensured_tree, updated_map} <- | |
| load_page_from_path(page_path, current_group, current_user, pages_tree_map) do | |
| {:ok, | |
| socket | |
| |> maybe_subscribe_page(current_group, page) | |
| |> maybe_subscribe_backlinks(page) | |
| |> assign_loaded_page(page, ensured_tree, updated_map)} | |
| end | |
| end | |
| `@impl` true | |
| def mount(%{"page_slug_segments" => page_slug_segments}, _session, socket) do | |
| page_path = page_slug_segments |> Enum.join("/") | |
| current_group = socket.assigns.ctx.current_group | |
| current_user = socket.assigns.current_user | |
| pages_tree_map = socket.assigns.ctx.pages_tree_map || %{} | |
| case load_page_from_path(page_path, current_group, current_user, pages_tree_map) do | |
| {:ok, page, ensured_tree, updated_map} -> | |
| {:ok, | |
| socket | |
| |> maybe_subscribe_page(current_group, page) | |
| |> maybe_subscribe_backlinks(page) | |
| |> assign_loaded_page(page, ensured_tree, updated_map)} | |
| {:error, _reason} -> | |
| {:ok, | |
| socket | |
| |> put_flash(:error, "Page could not be loaded") | |
| |> push_navigate(to: ~p"/#{current_group.slug}/wiki")} | |
| end | |
| end |
| <button | ||
| id={@id} | ||
| form={@form} | ||
| type={@type} | ||
| class={[ | ||
| "aspect-square w-10", | ||
| if(@disabled, do: "opacity-20", else: "cursor-pointer"), | ||
| if(@hidden, do: "hidden"), | ||
| @tip && "tooltip", | ||
| @class | ||
| ]} | ||
| data-tip={@tip} | ||
| {@rest} |
There was a problem hiding this comment.
Add HTML disabled attribute to prevent clicks on disabled buttons.
The button only applies visual styling (opacity-20) when disabled, but doesn't set the HTML disabled attribute. This means users can still click and interact with "disabled" buttons, potentially triggering unintended actions.
Proposed fix
<button
id={`@id`}
form={`@form`}
type={`@type`}
+ disabled={`@disabled`}
class={[
"aspect-square w-10",
if(`@disabled`, do: "opacity-20", else: "cursor-pointer"),
if(`@hidden`, do: "hidden"),
`@tip` && "tooltip",
`@class`
]}
data-tip={`@tip`}
{`@rest`}
>📝 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.
| <button | |
| id={@id} | |
| form={@form} | |
| type={@type} | |
| class={[ | |
| "aspect-square w-10", | |
| if(@disabled, do: "opacity-20", else: "cursor-pointer"), | |
| if(@hidden, do: "hidden"), | |
| @tip && "tooltip", | |
| @class | |
| ]} | |
| data-tip={@tip} | |
| {@rest} | |
| <button | |
| id={`@id`} | |
| form={`@form`} | |
| type={`@type`} | |
| disabled={`@disabled`} | |
| class={[ | |
| "aspect-square w-10", | |
| if(`@disabled`, do: "opacity-20", else: "cursor-pointer"), | |
| if(`@hidden`, do: "hidden"), | |
| `@tip` && "tooltip", | |
| `@class` | |
| ]} | |
| data-tip={`@tip`} | |
| {`@rest`} |
🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/page_live/show/action_button.ex` around lines 17
- 29, The button only visually indicates disabled via CSS but still accepts
clicks; update the <button> element in the component (the element using attrs
id={`@id`}, form={`@form`}, type={`@type`}, class=[...], data-tip={`@tip`}, {`@rest`}) to
include the HTML disabled attribute when `@disabled` is truthy (e.g.,
disabled={`@disabled`}) and consider also adding aria-disabled={`@disabled`} for
accessibility so the button is non-interactive both functionally and
semantically.
| defmodule WikWeb.GroupLive.PageLive.Show.ModalUnsavedExit do | ||
| use WikWeb, :html |
There was a problem hiding this comment.
Filename typo: modal_usaved_exit.ex should be modal_unsaved_exit.ex.
The module name ModalUnsavedExit is correct, but the filename is missing the 'n' in "unsaved". This inconsistency violates Elixir's convention where filenames should match module names (snake_case).
🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/page_live/show/modal_usaved_exit.ex` around lines
1 - 2, The file is misnamed: rename the file from modal_usaved_exit.ex to
modal_unsaved_exit.ex so it matches the module
WikWeb.GroupLive.PageLive.Show.ModalUnsavedExit (Elixir expects snake_case
filenames matching module names); update any references/imports/path usages if
they refer to the old filename, then run mix compile to verify there are no
remaining mismatches.
| <.form | ||
| :if={Ash.can?({@page, :update}, @current_user)} | ||
| class="grid grid-cols-[1fr_auto] gap-2" | ||
| for={:page_tree_title} | ||
| phx-change="page_title_change" | ||
| phx-submit="page_title_apply" | ||
| > | ||
| <input | ||
| type="text" | ||
| name="title" | ||
| autocomplete="off" | ||
| value={Phoenix.HTML.Form.normalize_value("text", @input)} | ||
| class="text-3xl w-full mb-8 bg-base-300/50 px-1" | ||
| /> | ||
| <div class=""> | ||
| <button | ||
| type="submit" | ||
| class="btn btn-square hover:btn-accent" | ||
| phx-click={ | ||
| JS.toggle(to: ".pagehead-form") | ||
| |> JS.toggle(to: ".pagehead-h1") | ||
| } | ||
| > | ||
| <.icon name="hero-check" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="btn btn-square hover:btn-accent" | ||
| phx-click={ | ||
| JS.push("page_title_cancel") | ||
| |> JS.toggle(to: ".pagehead-form") | ||
| |> JS.toggle(to: ".pagehead-h1") | ||
| } | ||
| > | ||
| <.icon name="hero-x-mark" /> | ||
| </button> | ||
| </div> | ||
| </.form> |
There was a problem hiding this comment.
Form is hidden but UI breaks when user lacks update permission.
When Ash.can?({@page, :update}, @current_user) is false, the form doesn't render, but the H1 toggle still tries to show .pagehead-form. The user clicks the title, the H1 hides, but nothing appears because there's no form.
Suggested fix to disable toggle when not editable
<h1
- class="pagehead-h1 text-3xl mb-8 hover:bg-base-300/20 cursor-pointer"
- phx-click={JS.toggle() |> JS.toggle(to: ".pagehead-form")}
+ class={"pagehead-h1 text-3xl mb-8 #{if Ash.can?({`@page`, :update}, `@current_user`), do: "hover:bg-base-300/20 cursor-pointer", else: ""}"}
+ phx-click={if Ash.can?({`@page`, :update}, `@current_user`), do: JS.toggle() |> JS.toggle(to: ".pagehead-form"), else: nil}
>
{`@input`}
</h1>Or pass an @editable assign to control this behavior.
🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/page_live/show/page_head.ex` around lines 16 -
54, The H1 toggle still triggers JS.toggle(to: ".pagehead-form") even when the
edit form (the .form guarded by Ash.can?({`@page`, :update}, `@current_user`)) is
not rendered; update the template to conditionally enable the toggle only when
editable (e.g., add an `@editable` assign derived from Ash.can? and use it to
guard the phx-click/JS.toggle calls on the H1 and buttons) or render a
non-interactive title when not editable. Ensure the references to JS.toggle(to:
".pagehead-form") and JS.toggle(to: ".pagehead-h1") are only emitted when
`@editable` is true so clicking the title won’t hide the H1 without showing a
form.
| <li :for={backlink <- @backlinks} class="text-sm"> | ||
| <% source_path = page_tree_path_for(@ctx, backlink.source_page_id) %> | ||
| <.link | ||
| navigate={WikWeb.GroupLive.PageLive.Show.page_url(@ctx.current_group, source_path)} | ||
| class="opacity-70 hover:opacity-100 transition" | ||
| > | ||
| {backlink_label(@ctx, backlink)} | ||
| </.link> | ||
| </li> |
There was a problem hiding this comment.
Potential broken navigation when source_path is nil.
If page_tree_path_for returns nil (line 11), the link on line 13 will call page_url(@ctx.current_group, nil), which may produce an invalid URL. Consider either filtering out backlinks with unresolvable paths or rendering them differently (e.g., as non-clickable text).
Proposed fix: filter or conditionally render
<li :for={backlink <- `@backlinks`} class="text-sm">
<% source_path = page_tree_path_for(`@ctx`, backlink.source_page_id) %>
- <.link
- navigate={WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, source_path)}
- class="opacity-70 hover:opacity-100 transition"
- >
- {backlink_label(`@ctx`, backlink)}
- </.link>
+ <%= if source_path do %>
+ <.link
+ navigate={WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, source_path)}
+ class="opacity-70 hover:opacity-100 transition"
+ >
+ {backlink_label(`@ctx`, backlink)}
+ </.link>
+ <% else %>
+ <span class="opacity-50">{backlink_label(`@ctx`, backlink)}</span>
+ <% end %>
</li>📝 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.
| <li :for={backlink <- @backlinks} class="text-sm"> | |
| <% source_path = page_tree_path_for(@ctx, backlink.source_page_id) %> | |
| <.link | |
| navigate={WikWeb.GroupLive.PageLive.Show.page_url(@ctx.current_group, source_path)} | |
| class="opacity-70 hover:opacity-100 transition" | |
| > | |
| {backlink_label(@ctx, backlink)} | |
| </.link> | |
| </li> | |
| <li :for={backlink <- `@backlinks`} class="text-sm"> | |
| <% source_path = page_tree_path_for(`@ctx`, backlink.source_page_id) %> | |
| <%= if source_path do %> | |
| <.link | |
| navigate={WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, source_path)} | |
| class="opacity-70 hover:opacity-100 transition" | |
| > | |
| {backlink_label(`@ctx`, backlink)} | |
| </.link> | |
| <% else %> | |
| <span class="opacity-50">{backlink_label(`@ctx`, backlink)}</span> | |
| <% end %> | |
| </li> |
🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/page_live/show/panels/Backlinks.ex` around lines
10 - 18, The backlink rendering can produce an invalid URL when
page_tree_path_for(`@ctx`, backlink.source_page_id) returns nil; update the
rendering in the Backlinks component to either filter out such backlinks or
conditionally render them as non-clickable text: compute source_path via
page_tree_path_for for each backlink and if source_path is nil render
backlink_label(`@ctx`, backlink) inside a <li> as plain text (or add an
Enum.filter over `@backlinks` to drop items where page_tree_path_for returns nil)
otherwise render the existing .link that calls
WikWeb.GroupLive.PageLive.Show.page_url(`@ctx.current_group`, source_path).
| defp ensure_not_descendant(_node_path, ""), do: :ok | ||
|
|
||
| defp ensure_not_descendant(node_path, new_parent_path) do | ||
| if String.starts_with?(new_parent_path, node_path) do | ||
| {:error, :invalid_target} | ||
| else | ||
| :ok | ||
| end | ||
| end |
There was a problem hiding this comment.
Fix false-positive descendant check.
The current check String.starts_with?(new_parent_path, node_path) produces false positives. Moving "foo" under "foobar" would incorrectly fail because "foobar" starts with "foo", even though "foobar" is not a descendant of "foo".
The check should verify the path boundary by appending "/":
🐛 Proposed fix
defp ensure_not_descendant(_node_path, ""), do: :ok
defp ensure_not_descendant(node_path, new_parent_path) do
- if String.starts_with?(new_parent_path, node_path) do
+ if String.starts_with?(new_parent_path, node_path <> "/") do
{:error, :invalid_target}
else
:ok
end
end📝 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.
| defp ensure_not_descendant(_node_path, ""), do: :ok | |
| defp ensure_not_descendant(node_path, new_parent_path) do | |
| if String.starts_with?(new_parent_path, node_path) do | |
| {:error, :invalid_target} | |
| else | |
| :ok | |
| end | |
| end | |
| defp ensure_not_descendant(_node_path, ""), do: :ok | |
| defp ensure_not_descendant(node_path, new_parent_path) do | |
| if String.starts_with?(new_parent_path, node_path <> "/") do | |
| {:error, :invalid_target} | |
| else | |
| :ok | |
| end | |
| end |
🤖 Prompt for AI Agents
In `@lib/wik_web/live/group_live/wikitree_live.ex` around lines 126 - 134, The
descendant check in ensure_not_descendant incorrectly treats prefixes as
descendants (e.g., "foo" vs "foobar"); change the starts_with? test to enforce a
path boundary by checking String.starts_with?(new_parent_path, node_path <> "/")
so only true descendants (with a "/" after the node_path) are rejected, leaving
the existing clause for empty new_parent_path intact.
| def notify(%Ash.Notifier.Notification{action: %{type: type}} = notification) | ||
| when type in @tree_events do | ||
| group_id = extract_group_id(notification) | ||
|
|
||
| case Wik.Wiki.PageTree.Utils.pages_tree_topic(group_id) do | ||
| nil -> | ||
| :ok | ||
|
|
||
| topic -> | ||
| Phoenix.PubSub.broadcast(Wik.PubSub, topic, {:pages_tree_updated, group_id}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Guard against group_id == nil before deriving topic/broadcasting.
Right now extract_group_id/1 can return nil, and pages_tree_topic(nil) may crash depending on its implementation.
Proposed fix
def notify(%Ash.Notifier.Notification{action: %{type: type}} = notification)
when type in `@tree_events` do
group_id = extract_group_id(notification)
- case Wik.Wiki.PageTree.Utils.pages_tree_topic(group_id) do
- nil ->
- :ok
-
- topic ->
- Phoenix.PubSub.broadcast(Wik.PubSub, topic, {:pages_tree_updated, group_id})
- end
+ with group_id when is_binary(group_id) and group_id != "" <- group_id,
+ topic when is_binary(topic) and topic != "" <- Wik.Wiki.PageTree.Utils.pages_tree_topic(group_id) do
+ Phoenix.PubSub.broadcast(Wik.PubSub, topic, {:pages_tree_updated, group_id})
+ else
+ _ -> :ok
+ end
end🤖 Prompt for AI Agents
In `@lib/wik/notifiers/page_tree_mutation.ex` around lines 6 - 17, The notify/1
function calls extract_group_id(notification) and then immediately passes the
result to Wik.Wiki.PageTree.Utils.pages_tree_topic/1 which can crash if group_id
is nil; guard against that by checking group_id is not nil before deriving topic
or broadcasting (either add a when not is_nil(group_id) clause to notify/1 or an
explicit early return when extract_group_id(notification) == nil), then only
call pages_tree_topic(group_id) and Phoenix.PubSub.broadcast when group_id is
non-nil.
Summary by CodeRabbit
New Features
/docs/api/overview)[[docs/api]]) for clearer page referencesUI Updates
✏️ Tip: You can customize this high-level summary in your review settings.