diff --git a/db/migrate/20260823193530_backfill_plan_slugs.co_plan.rb b/db/migrate/20260823193530_backfill_plan_slugs.co_plan.rb new file mode 100644 index 00000000..a77b7786 --- /dev/null +++ b/db/migrate/20260823193530_backfill_plan_slugs.co_plan.rb @@ -0,0 +1,251 @@ +# This migration comes from co_plan (originally 20260823000000) +class BackfillPlanSlugs < ActiveRecord::Migration[8.1] + # Gives every plan the readable leaf segment of its address, and makes + # a plan without one unrepresentable. + # + # AddPlanSlugsAndUrlAliases deliberately left slugs NULL and let the app + # fill them in on the next save. The cost turned out to be a permanent + # second address: a plan nobody has re-saved since has no readable path + # at all, so every link to it — and its rel=canonical — falls back to + # /plans/. A document has one address, so the backfill has to + # actually happen. + # + # The slug rules are inlined below rather than called through + # CoPlan::Slug and Plans::AssignSlug, for the same reason the folder and + # handle backfills inline theirs: a migration has to keep producing the + # same result years from now, even after the app's rules move on. + # + # No aliases are recorded. These plans never had a readable address, so + # there's no old path anyone could be holding — the legacy /plans/ + # route is what their existing links go through, and it 301s onward. + + # Mirrors CoPlan::Slug. + MAX_LENGTH = 60 + NOISE_TOKENS = %w[plan plans doc document].freeze + # Mirrors Plans::AssignSlug. Unambiguous alphabet — no 0/o/1/l. + SUFFIX_ALPHABET = "23456789abcdefghjkmnpqrstuvwxyz".freeze + SUFFIX_LENGTH = 4 + + def up + backfill_plan_slugs + change_column_null :coplan_plans, :slug, false + end + + def down + change_column_null :coplan_plans, :slug, true + end + + private + + # Two passes. The first registers the slugs already in use at each level + # so the backfill can't collide with them; the second assigns the rest, + # oldest first, so the earliest plan keeps the clean un-suffixed segment. + def backfill_plan_slugs + folders = load_folders + libraries = load_library_handles + plan_types = load_plan_type_names + locations = load_plan_locations + creator_libraries = load_creator_libraries + + plans = connection.select_all(<<~SQL.squish).to_a + SELECT id, title, slug, slug_suffix, plan_type_id, created_by_user_id, created_at + FROM coplan_plans + ORDER BY created_at, id + SQL + + # Sibling folder slugs are taken too: Urls::Resolve hands a contested + # segment to the folder, so a plan sharing one would have no reachable + # address. Folders never take a suffix; plans do. + taken = {} + folders.each_value do |folder| + key = [ folder["library_id"], folder["parent_id"] ] + (taken[key] ||= Set.new) << folder["slug"] + end + + pending = [] + plans.each do |plan| + library_id, folder_id = location_for(plan, locations, creator_libraries) + key = [ library_id, folder_id ] + if plan["slug"].present? + seen = (taken[key] ||= Set.new) + # Only an un-suffixed slug blocks the segment; a suffixed one has + # already moved out of the way. Its full leaf is still spoken for + # though, so it goes in too — otherwise a backfilled plan whose + # deterministic candidate happens to match would be handed the + # same address, and nothing downstream would reject it. + if plan["slug_suffix"].blank? + seen << plan["slug"] + else + seen << "#{plan['slug']}~#{plan['slug_suffix']}" + end + next + end + pending << [ plan, key, folder_id, library_id ] + end + + say "Backfilling slugs for #{pending.size} plan(s)" if pending.any? + pending.each do |plan, key, folder_id, library_id| + slug = derive_slug( + plan["title"], + redundant_phrases(library_id, folder_id, plan["plan_type_id"], folders, libraries, plan_types) + ) + seen = (taken[key] ||= Set.new) + if seen.include?(slug) + suffix = unique_suffix(plan["id"], slug, seen) + write_slug(plan["id"], slug, suffix) + seen << "#{slug}~#{suffix}" + else + write_slug(plan["id"], slug, nil) + seen << slug + end + end + end + + def write_slug(plan_id, slug, suffix) + execute <<~SQL.squish + UPDATE coplan_plans + SET slug = #{quote(slug)}, slug_suffix = #{suffix ? quote(suffix) : "NULL"} + WHERE id = #{quote(plan_id)} + SQL + end + + # Where the plan lives: its placement when it has one, otherwise the + # root of its author's library — which is exactly what Plan#library + # resolves to, and what its URL says. + def location_for(plan, locations, creator_libraries) + placement = locations[plan["id"]] + return [ placement["library_id"], placement["folder_id"] ] if placement + + [ creator_libraries[plan["created_by_user_id"]], nil ] + end + + # Everything the path already spells out: the handle, every folder on + # the way down, and the plan type. + def redundant_phrases(library_id, folder_id, plan_type_id, folders, libraries, plan_types) + names = [] + folder_id_walk = folder_id + while folder_id_walk && (folder = folders[folder_id_walk]) + names.unshift(folder["name"]) + folder_id_walk = folder["parent_id"] + end + + [ libraries[library_id], *names, plan_types[plan_type_id] ].compact_blank + end + + # --- The slug rules, inlined --------------------------------------- + + def derive_slug(title, phrases) + full = strip_noise(slug_tokens(title)) + return "untitled" if full.empty? + + truncate(strip_redundancy(full, phrases).join("-")) + end + + def slugify(text) + normalize(text.to_s.unicode_normalize(:nfc).downcase.gsub(/[^[[:alnum:]]]+/, "-")) + end + + def normalize(hyphenated) + truncate(hyphenated.gsub(/-{2,}/, "-").delete_prefix("-").delete_suffix("-")) + end + + def truncate(slug) + return slug if slug.length <= MAX_LENGTH + + slug[0, MAX_LENGTH].rpartition("-").first.presence || slug[0, MAX_LENGTH] + end + + def slug_tokens(text) + slugify(text).split("-") + end + + def compare_key(text) + slugify(text).delete("-") + end + + def strip_noise(tokens) + kept = tokens.dup + kept.shift while kept.size > 1 && NOISE_TOKENS.include?(kept.first) + kept.pop while kept.size > 1 && NOISE_TOKENS.include?(kept.last) + kept.presence || tokens + end + + # Repeats until nothing more comes off, so "Orders LiveOrder Cart" + # under /orders/liveorder loses both leading words regardless of the + # order they appear in. + def strip_redundancy(tokens, phrases) + kept = tokens + loop do + before = kept + phrases.each { |phrase| kept = strip_leading(kept, phrase) } + break if kept == before + end + kept.presence || tokens + end + + # Never strips everything — a plan titled exactly "LiveOrder" inside + # "LiveOrder" keeps its name rather than becoming empty. + def strip_leading(tokens, phrase) + key = compare_key(phrase) + return tokens if key.blank? + + (1...tokens.length).each do |n| + return tokens.drop(n) if tokens.first(n).join == key + end + tokens + end + + # Derived from the plan id rather than random: re-running this migration + # on the same data has to produce the same URLs. + def unique_suffix(plan_id, slug, seen) + attempt = 0 + loop do + candidate = suffix_for(plan_id, attempt) + return candidate unless seen.include?("#{slug}~#{candidate}") + + attempt += 1 + end + end + + def suffix_for(plan_id, attempt) + digest = Digest::SHA256.hexdigest("#{plan_id}:#{attempt}").to_i(16) + SUFFIX_LENGTH.times.map do |index| + SUFFIX_ALPHABET[(digest >> (index * 8)) % SUFFIX_ALPHABET.length] + end.join + end + + # --- Reads --------------------------------------------------------- + + def load_folders + connection.select_all(<<~SQL.squish).to_a.index_by { |row| row["id"] } + SELECT id, library_id, parent_id, name, slug FROM coplan_folders + SQL + end + + def load_library_handles + connection.select_all("SELECT id, handle FROM coplan_libraries").to_a + .to_h { |row| [ row["id"], row["handle"] ] } + end + + def load_plan_type_names + connection.select_all("SELECT id, name FROM coplan_plan_types").to_a + .to_h { |row| [ row["id"], row["name"] ] } + end + + def load_plan_locations + connection.select_all(<<~SQL.squish).to_a.index_by { |row| row["plan_id"] } + SELECT plan_id, library_id, folder_id FROM coplan_plan_placements + SQL + end + + def load_creator_libraries + connection.select_all(<<~SQL.squish).to_a + SELECT owner_id, id FROM coplan_libraries WHERE owner_type = 'CoPlan::User' + SQL + .to_h { |row| [ row["owner_id"], row["id"] ] } + end + + def quote(value) + connection.quote(value) + end +end diff --git a/db/schema.rb b/db/schema.rb index 5322621f..279cde0f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_21_205749) do +ActiveRecord::Schema[8.1].define(version: 2026_08_23_193530) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" @@ -345,7 +345,7 @@ t.json "metadata" t.string "plan_type_id", limit: 36, null: false t.text "search_text", size: :medium - t.string "slug" + t.string "slug", null: false t.string "slug_suffix", limit: 8 t.text "summary" t.string "summary_content_sha256", limit: 64 diff --git a/engine/app/controllers/coplan/api/v1/base_controller.rb b/engine/app/controllers/coplan/api/v1/base_controller.rb index 7c96d0be..97faf507 100644 --- a/engine/app/controllers/coplan/api/v1/base_controller.rb +++ b/engine/app/controllers/coplan/api/v1/base_controller.rb @@ -118,6 +118,14 @@ def api_token_id @api_token&.id end + # A document's address, absolute, so a caller can hand it straight to + # a human. Built from the request rather than from Urls::Canonical + # alone: only the request knows the host, and a host that mounted the + # engine under a prefix needs that prefix on the front. + def plan_web_url(plan) + "#{request.base_url}#{root_path.chomp("/")}#{Urls::Canonical.plan_path(plan)}" + end + def set_plan @plan = CoPlan::Plan.find_by(id: params[:plan_id] || params[:id]) unless @plan diff --git a/engine/app/controllers/coplan/api/v1/plans_controller.rb b/engine/app/controllers/coplan/api/v1/plans_controller.rb index 3046b410..22847be1 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -7,7 +7,13 @@ class PlansController < BaseController def index plans = Plan - .includes(:plan_type, :created_by_user) + # Where each plan lives, preloaded whole. Two fields need it and + # each needs more of it than it looks: `folder_path` walks the + # folder's ancestors, and `url` walks those *and* the library for + # its handle. Left to the associations that's several queries a + # plan on a list endpoint agents page through. + .includes(:plan_type, :created_by_user, + placement: [ :library, { folder: { parent: :parent } } ]) .visible_to(current_user) .order(updated_at: :desc) plans = apply_index_filters(plans) @@ -18,9 +24,6 @@ def index plans = plans.joins(:placement) .where(coplan_plan_placements: { folder_id: params[:folder_id] }) end - @placements = PlanPlacement.where(plan_id: plans.map(&:id)) - .includes(folder: { parent: :parent }) - .index_by(&:plan_id) render json: plans.map { |p| plan_json(p) } end @@ -402,23 +405,19 @@ def resolve_folder_params end end - # Where the plan lives. Used to be viewer-relative — the caller's - # own shelf — but a plan is filed in exactly one place now, so - # every caller gets the same answer. One query per call; index - # batches placements up front via @placements. - def placement_for(plan) - if defined?(@placements) && @placements - @placements[plan.id] - else - plan.placement - end - end - def plan_json(plan) - placement = placement_for(plan) + # Where the plan lives. Used to be viewer-relative — the caller's + # own shelf — but a plan is filed in exactly one place now, so + # every caller gets the same answer. The list endpoint preloads + # this; single-plan responses take the one query. + placement = plan.placement { id: plan.id, title: plan.title, + # The document's address — the one a caller should hand to a + # human. An agent that files a plan and then says where it went + # has to be able to name it, and the id form isn't the name. + url: plan_web_url(plan), visibility: plan.visibility, archived: plan.archived?, archived_at: plan.archived_at, diff --git a/engine/app/controllers/coplan/browse_controller.rb b/engine/app/controllers/coplan/browse_controller.rb index aee14907..a114da87 100644 --- a/engine/app/controllers/coplan/browse_controller.rb +++ b/engine/app/controllers/coplan/browse_controller.rb @@ -5,26 +5,56 @@ module CoPlan # /sam Sam, and Sam's library # /sam/liveorder a folder # /sam/liveorder/cart-roadmap a document + # /sam/liveorder/cart-roadmap/edit the document's editor # # Every prefix is a real page, so trimming a segment off any URL walks - # you up the tree. One action serves all three because they are one - # thing — a place in a library — and which of the three a path names - # isn't knowable until the segments are resolved against the database. + # you up the tree. One action serves all of them because they are one + # thing — a place in a library, and the pages belonging to what's there + # — and which of them a path names isn't knowable until the segments are + # resolved against the database. # # Inherits PlansController to reuse the workspace index and the document - # view wholesale rather than duplicating (or prematurely extracting) + # views wholesale rather than duplicating (or prematurely extracting) # ~250 lines of interdependent loading. The action is named `browse`, not # `show`, so the inherited `before_action :set_plan, only: [:show, ...]` # doesn't fire on a path that has no plan id in it. class BrowseController < PlansController + # A document's sub-pages: the `page` a route supplied, mapped to the + # inherited action that renders it and the template it renders. + # Anything not in here is a plain document page. + PAGES = { + "edit" => { action: :edit_content, template: "coplan/plans/edit_content" }, + "history" => { action: :history, template: "coplan/plans/history" }, + "version" => { action: :version, template: "coplan/plan_versions/show" }, + # A bare fragment: the history page loads it into a turbo-frame. + "version_diff" => { action: :version_diff, template: "coplan/plan_versions/diff", layout: false } + }.freeze + def browse - result = Urls::Resolve.call(handle: params[:handle], slug_path: params[:slug_path]) + result = resolve + + # A tail only names an action when it hangs off a document. On + # `/sam/notes/history`, where "notes" is a folder, "history" is a + # document slug (or nothing) — so put the path back together and + # resolve it as a place. + if page && !result.plan + # Reassembled before `page` goes, because the tail is derived from + # it: delete first and there's nothing left to put back, so the + # folder resolves a second time and a plan actually named "edit" + # never gets found. + whole_path = [ params[:slug_path], *page_tail ].compact_blank.join("/") + params.delete(:page) + result = resolve(slug_path: whole_path) + end return head :not_found unless result.found? # A stale-but-recognizable path: 301 so the address bar, and - # everything copied out of it, converges on the current URL. + # everything copied out of it, converges on the current URL. The + # sub-page tail rides along — someone who asked for a moved + # document's editor wants the editor, not the document. if result.redirect_to_path.present? - return redirect_to path_to_url(result.redirect_to_path), status: :moved_permanently + return redirect_to path_to_url([ result.redirect_to_path, *page_tail ].compact_blank.join("/")), + status: :moved_permanently end result.plan ? render_plan(result.plan) : render_library(result.library, result.folder) @@ -32,11 +62,32 @@ def browse private + def resolve(slug_path: params[:slug_path]) + Urls::Resolve.call(handle: params[:handle], slug_path: slug_path) + end + + def page + PAGES[params[:page]] + end + + # The segments a sub-page route consumed, so a path that turns out not + # to name one can be reassembled. + def page_tail + case params[:page] + when "edit" then [ "edit" ] + when "history" then [ "history" ] + when "version" then [ "history", params[:revision] ] + when "version_diff" then [ "history", params[:revision], "diff" ] + else [] + end + end + def render_plan(plan) @plan = plan authorize!(@plan, :show?) - show - render "coplan/plans/show" unless performed? + target = page || { action: :show, template: "coplan/plans/show" } + send(target[:action]) + render target[:template], layout: target.fetch(:layout, true) unless performed? end # Every library renders the same page. What you can do to what's in it @@ -46,12 +97,12 @@ def render_plan(plan) # library feel like a different, lesser app: no filters, no folder # counts, no "since you last looked". # - # `index` reads the folder from params, so the resolved folder is - # handed over the same way the legacy ?folder= form supplied it. + # The resolved folder is handed to `index` directly: a folder is a + # place with an address, so it never travels as a query param. def render_library(library, folder) authorize!(library, :show?) @library = library - params[:folder] = folder&.id + @folder = folder index render "coplan/plans/index" unless performed? end diff --git a/engine/app/controllers/coplan/folders_controller.rb b/engine/app/controllers/coplan/folders_controller.rb index 724f0d99..5b78d6ba 100644 --- a/engine/app/controllers/coplan/folders_controller.rb +++ b/engine/app/controllers/coplan/folders_controller.rb @@ -51,7 +51,7 @@ def create if parent.nil? # Don't silently create a root folder when the chosen parent has # since been deleted (matches the API's unknown-parent handling). - redirect_back fallback_location: plans_path, + redirect_back fallback_location: helpers.own_library_browse_path(current_user), alert: "Couldn't create folder: the parent folder no longer exists." return end @@ -69,9 +69,11 @@ def create library: library, actor: current_user, event_type: "folder_created", folder: folder, after: folder.path ) - redirect_to plans_path(folder: folder.id), notice: "Folder “#{folder.name}” created." + # Straight to the new folder's own address — it's a place now, so + # that's where "created" lands. + redirect_to helpers.folder_browse_path(folder), notice: "Folder “#{folder.name}” created." else - redirect_back fallback_location: plans_path, + redirect_back fallback_location: helpers.own_library_browse_path(current_user), alert: "Couldn't create folder: #{folder.errors.full_messages.join(", ")}" end end diff --git a/engine/app/controllers/coplan/home_controller.rb b/engine/app/controllers/coplan/home_controller.rb index da635a7b..1141df49 100644 --- a/engine/app/controllers/coplan/home_controller.rb +++ b/engine/app/controllers/coplan/home_controller.rb @@ -1,11 +1,16 @@ module CoPlan # Home — the org-facing "what's happening" surface. A per-plan-per-day # activity feed over published work, plus the sitewide search in the nav - # as the other discovery tool. Your own working list lives in the - # Workspace (/plans); Home is everyone's. + # as the other discovery tool. Your own working list lives in your + # library, at /; Home is everyone's. + # + # `?tag=` narrows it to one tag. Every cross-library list of plans lands + # here: a list that spans libraries isn't a place inside one, which is + # why it doesn't have a path in anybody's. class HomeController < ApplicationController def show - @items = HomeFeed.build + @tag = params[:tag].presence + @items = HomeFeed.build(tag: @tag) @items_by_date = @items.group_by(&:date) end end diff --git a/engine/app/controllers/coplan/notifications_controller.rb b/engine/app/controllers/coplan/notifications_controller.rb index 0cc3ebca..c3917ec1 100644 --- a/engine/app/controllers/coplan/notifications_controller.rb +++ b/engine/app/controllers/coplan/notifications_controller.rb @@ -85,7 +85,7 @@ def mark_plan_read turbo_stream.remove(helpers.plan_unread_badge_id(plan_id)) ] end - format.html { redirect_back fallback_location: plans_path, notice: "Notifications cleared." } + format.html { redirect_back fallback_location: helpers.own_library_browse_path(current_user), notice: "Notifications cleared." } end end diff --git a/engine/app/controllers/coplan/plan_versions_controller.rb b/engine/app/controllers/coplan/plan_versions_controller.rb deleted file mode 100644 index 819e1f9c..00000000 --- a/engine/app/controllers/coplan/plan_versions_controller.rb +++ /dev/null @@ -1,33 +0,0 @@ -module CoPlan - class PlanVersionsController < ApplicationController - before_action :set_plan - before_action :set_version, only: [ :show, :diff ] - before_action :set_diff, only: [ :show, :diff ] - - def show - authorize!(@plan, :show?) - end - - def diff - authorize!(@plan, :show?) - render layout: false - end - - private - - def set_plan - @plan = Plan.find(params[:plan_id]) - end - - def set_version - @version = @plan.plan_versions.find(params[:id]) - end - - def set_diff - @previous_version = @plan.plan_versions.find_by(revision: @version.revision - 1) - if @previous_version - @diff = Diffy::Diff.new(@previous_version.content_markdown, @version.content_markdown, include_plus_and_minus_in_html: true, context: 3) - end - end - end -end diff --git a/engine/app/controllers/coplan/plans_controller.rb b/engine/app/controllers/coplan/plans_controller.rb index ee7aed52..7e49f03f 100644 --- a/engine/app/controllers/coplan/plans_controller.rb +++ b/engine/app/controllers/coplan/plans_controller.rb @@ -1,6 +1,6 @@ module CoPlan class PlansController < ApplicationController - before_action :set_plan, only: [ :show, :edit, :update, :publish, :hide, :archive, :unarchive, :move_to_folder, :toggle_checkbox, :history, :edit_content, :update_content, :preview ] + before_action :set_plan, only: [ :show, :update, :publish, :hide, :archive, :unarchive, :move_to_folder, :toggle_checkbox, :update_content, :preview ] # /plans/ is the legacy address; the readable one is canonical. # `only: [ :show ]` matters twice over — it's also why BrowseController, # which calls `show` as a method from its own action, doesn't bounce @@ -9,9 +9,6 @@ class PlansController < ApplicationController PER_PAGE = 20 - SCOPES = %w[mine all].freeze - DEFAULT_SCOPE = "mine".freeze - # "private" is the user-facing name; "draft" is the stored visibility # value and stays accepted so old links keep working. FILTERS = %w[published draft private archived].freeze @@ -30,26 +27,21 @@ class PlansController < ApplicationController # Turbo-frame requests are page fetches for one of those lists and # render only the row page partial (`group` param: "level" pages the # direct-placement list, anything else the flat results). + # + # `@library` and `@folder` say which place is being browsed, and both + # arrive from BrowseController, which resolved them from the path. + # There is no folder param: a folder is a place, and places have + # addresses — a folder that no longer exists no longer resolves, so + # "that folder is gone" is a 404 on the way in rather than a check + # here. def index - @scope = SCOPES.include?(params[:scope]) ? params[:scope] : DEFAULT_SCOPE @filter = FILTERS.include?(params[:filter]) ? params[:filter] : nil @filter = "draft" if @filter == "private" @updated_window = UPDATED_WINDOWS[params[:updated]] && params[:updated] load_folder_tree - if params[:folder].present? - @folder = @folders_by_id[params[:folder]] - if @folder.nil? && !turbo_frame_request? - redirect_to plans_path(params.permit(:scope, :filter, :plan_type, :tag, :updated).to_h), - alert: "That folder no longer exists." - return - end - end - - plans = scoped_plans_base.includes(:plan_type, :tags, :created_by_user, :current_version_stub) + plans = library_plans.includes(:plan_type, :tags, :created_by_user, :current_version_stub) plans = apply_workspace_filters(plans) - # Stale frame fetch for a since-deleted folder: render an empty page. - plans = plans.none if params[:folder].present? && @folder.nil? if filtered_view? || turbo_frame_request? plans = if params[:group] == "level" || (!filtered_view? && turbo_frame_request?) @@ -77,7 +69,7 @@ def index has_next_page: @has_next_page, group_key: params[:group].presence || "results", frame_filter: @filter, - frame_folder: params[:folder].presence + frame_folder: @folder&.id }, layout: false return @@ -114,7 +106,7 @@ def move_to_folder unless folder respond_to do |format| format.json { render json: { error: "Unknown folder" }, status: :unprocessable_content } - format.html { redirect_back fallback_location: plans_path, alert: "Unknown folder." } + format.html { redirect_back fallback_location: helpers.own_library_browse_path(current_user), alert: "Unknown folder." } end return end @@ -124,7 +116,7 @@ def move_to_folder unless result.success? respond_to do |format| format.json { render json: { error: result.error }, status: :unprocessable_content } - format.html { redirect_back fallback_location: plans_path, alert: result.error } + format.html { redirect_back fallback_location: helpers.own_library_browse_path(current_user), alert: result.error } end return end @@ -138,7 +130,7 @@ def move_to_folder message: notice } end - format.html { redirect_back fallback_location: plans_path, notice: notice } + format.html { redirect_back fallback_location: helpers.own_library_browse_path(current_user), notice: notice } end end @@ -146,7 +138,7 @@ def show authorize!(@plan, :show?) # Old ?tab=history links: history is its own page now (the other # former tabs are same-page sections). - return redirect_to history_plan_path(@plan) if params[:tab] == "history" + return redirect_to helpers.plan_history_browse_path(@plan) if params[:tab] == "history" # Where the plan lives. One placement, the same for every reader — # it drives the compact jump up to the containing folder. @placement = PlanPlacement.includes(:library, folder: { parent: :parent }) @@ -166,17 +158,26 @@ def show end # A full page (reached from the header's clock icon), not a tab — - # Backspace or the back link returns to the document. + # Backspace or the back link returns to the document. Lives at + # /history; the actions below are the pages under it. def history authorize!(@plan, :show?) @history_items = @plan.history_items end - # The separate title-and-tags page merged into the unified editor — - # keep the route working for old links. - def edit - authorize!(@plan, :update?) - redirect_to edit_content_plan_path(@plan) + # One past version, addressed by the revision number the history list + # shows — /history/7. + def version + authorize!(@plan, :show?) + load_version + end + + # The same comparison as a bare fragment, for the turbo-frame that the + # history page loads it into (rendered without a layout — see + # BrowseController::PAGES). + def version_diff + authorize!(@plan, :show?) + load_version end def update @@ -493,12 +494,12 @@ def folder_ancestry(folder) # PlanViewer.last_seen_at — recency against your own reading history, # not workflow state. Bounded: only the newest RECENT_CANDIDATES are # considered, and at most RECENT_LIMIT surface. - # Follows the active scope, so "mine" stays your own work. That means - # the "new to you" badge only fires under scope=all: it needs plans - # someone else wrote, and those used to reach your workspace by being - # filed onto your shelf. With one place per plan, they don't. + # Scoped to the library being browsed, which is why the "new to you" + # badge fires on someone else's page rather than your own: it needs + # plans another person wrote, and with one place per plan those live on + # their shelf, not yours. def load_recently_updated - candidates = scoped_plans_base.active + candidates = library_plans.active .includes(:created_by_user) .order(updated_at: :desc, id: :desc) .limit(RECENT_CANDIDATES) @@ -557,14 +558,6 @@ def unread_counts_for(plans) # The base relation for this view. Used by both the main-pane plan # lists and the sidebar counts, so folder/tag counts always match what # clicking through shows. - def scoped_plans_base - # Everything you can see, wherever it lives — the one view that isn't - # a place. Reached by link (Home's tags), never from the sidebar. - return Plan.visible_to(current_user) if @scope == "all" - - library_plans - end - # Every document in the library being browsed, in either sense of "in": # filed into one of its folders, or loose at its root — its owner's own # work that isn't filed anywhere. @@ -584,8 +577,8 @@ def library_plans # derived in memory. # # `@library` is whatever the route resolved to — BrowseController sets - # it before delegating here. Your own is the default, which is what - # /_/plans and the legacy /plans both mean. + # it before delegating here. Your own is the default, which is what the + # legacy /plans and /library both mean. def load_folder_tree @library ||= current_user.library @can_write = @library.writable_by?(current_user) @@ -621,11 +614,11 @@ def folder_subtree_ids(folder) # folder counts respect tag/type/date), INCLUDING the Hidden filter: # folder/tag/type links carry `filter` (WORKSPACE_LINK_PARAMS), so with # "Archived" active a folder count means "archived plans in here". All - # from scoped_plans_base, so other users' private plans never leak + # from library_plans, so other users' private plans never leak # through counts (Plan.visible_to). Without a filter, archived plans # are opt-in and excluded (filtered_plans defaults to .active). def load_workspace_sidebar - count_base = filtered_plans(scoped_plans_base, @filter) + count_base = filtered_plans(library_plans, @filter) direct_counts = apply_workspace_filters(count_base) .joins(:placement) @@ -724,21 +717,31 @@ def set_plan @plan = Plan.find(params[:id]) end + # The revision number in the URL, not a version id — see + # BrowseController's routes. A revision nobody wrote is a 404, the same + # as any other address that names nothing. + def load_version + @version = @plan.plan_versions.find_by!(revision: params[:revision]) + @previous_version = @plan.plan_versions.find_by(revision: @version.revision - 1) + return if @previous_version.nil? + + @diff = Diffy::Diff.new(@previous_version.content_markdown, @version.content_markdown, + include_plus_and_minus_in_html: true, context: 3) + end + # 301s /plans/ onto the document's readable address, so the # address bar — and everything anyone copies out of it — converges # there. Permanent rather than temporary: the id form isn't a # redirect-of-the-day, it's the old name for this page. # # HTML GETs only. A Turbo Frame fetch or a JSON caller asked for this - # exact URL and should get a response, not a hop. A plan whose slug - # hasn't been backfilled yet has no readable address to go to, so it - # renders here. + # exact URL and should get a response, not a hop. Every plan has a + # readable address to go to — `slug` is NOT NULL — so this always hops. def redirect_to_canonical_url return unless request.get? && request.format.html? return if turbo_frame_request? canonical = helpers.plan_browse_path(@plan) - return if canonical == plan_path(@plan) # The query string comes along: `?thread=` deep-links a comment # and `?tab=history` is itself a legacy hop onward. Dropping either diff --git a/engine/app/controllers/coplan/welcome_controller.rb b/engine/app/controllers/coplan/welcome_controller.rb index 58bddfc2..b7f532c9 100644 --- a/engine/app/controllers/coplan/welcome_controller.rb +++ b/engine/app/controllers/coplan/welcome_controller.rb @@ -3,7 +3,9 @@ module CoPlan # # Behavior at "/" (root): # * Signed-in users who already have at least one plan are redirected to - # their Workspace — that's home base; the Feed is a nav click away. + # their own library — that's home base, and it has an address, so the + # session starts on the URL they'd want to share. The Feed is a nav + # click away. # * Everyone else (signed-in users with no plans yet, or anyone hitting the # page anonymously) sees the landing partial configured via # `CoPlan.configuration.landing_page_partial`. @@ -16,7 +18,8 @@ class WelcomeController < ApplicationController # page that needs to work for first-time visitors. We replace the engine's # required-auth `before_action` with a softer version that resolves the # current user when present (so we can personalize CTAs and redirect - # established users to /plans) but doesn't reject anonymous visitors. + # established users to their library) but doesn't reject anonymous + # visitors. # Hosts that gate the whole app at the perimeter (BeyondCorp, OIDC) will # still enforce sign-in upstream. skip_before_action :authenticate_coplan_user! @@ -24,7 +27,7 @@ class WelcomeController < ApplicationController def show if signed_in? && current_user.created_plans.exists? && params[:force].blank? - redirect_to plans_path and return + redirect_to helpers.own_library_browse_path(current_user) and return end @landing_partial = CoPlan.configuration.landing_page_partial diff --git a/engine/app/helpers/coplan/browse_helper.rb b/engine/app/helpers/coplan/browse_helper.rb index 14e322fe..7b6438c4 100644 --- a/engine/app/helpers/coplan/browse_helper.rb +++ b/engine/app/helpers/coplan/browse_helper.rb @@ -8,16 +8,23 @@ def library_browse_path(library, **options) browse_library_path(handle: library.handle, **options) end + # "Back to your own work" — where anything that needs somewhere to + # land goes when it has no place of its own to return to. This is the + # app's home base, and it's an address like any other. + def own_library_browse_path(user, **options) + library_browse_path(user.library, **options) + end + def folder_browse_path(folder, **options) browse_path(handle: folder.library.handle, slug_path: folder.slug_path, **options) end - # Falls back to the id form for a plan whose slug hasn't been - # backfilled yet — the migration leaves them NULL and lets the app - # fill them in on the next save, so both forms have to work meanwhile. + # A plan has exactly one address, so there is no id form to fall back + # to: `slug` is NOT NULL and every plan sits in exactly one library + # (see BackfillPlanSlugs), which makes `url_path` total. # - # Extra options (`thread:`, `anchor:`) ride along either way, so - # deep links don't have to know which form they got. + # Extra options (`thread:`, `anchor:`) ride along, so deep links keep + # working without knowing anything about the shape of the path. # # Built from the view's own route helpers rather than delegating to # Urls::Canonical: a host that mounts the engine somewhere other than @@ -25,17 +32,37 @@ def folder_browse_path(folder, **options) # outside a request have no way to know about it. def plan_browse_path(plan, **options) handle, slug_path = Urls::Canonical.split(plan.url_path) - return plan_path(plan, **options) if slug_path.blank? - browse_path(handle: handle, slug_path: slug_path, **options) end - # Absolute form, for rel=canonical. Returns nil rather than falling - # back: a canonical tag pointing at the id form would be claiming the - # ugly URL is the real one. + # Absolute form, for rel=canonical and anything that leaves the app. def plan_browse_url(plan) handle, slug_path = Urls::Canonical.split(plan.url_path) - slug_path && browse_url(handle: handle, slug_path: slug_path) + browse_url(handle: handle, slug_path: slug_path) + end + + # The document's own pages, addressed under the document. Same split as + # above, so they stay correct through a retitle or a move. + def plan_edit_browse_path(plan, **options) + handle, slug_path = Urls::Canonical.split(plan.url_path) + browse_edit_path(handle: handle, slug_path: slug_path, **options) + end + + def plan_history_browse_path(plan, **options) + handle, slug_path = Urls::Canonical.split(plan.url_path) + browse_history_path(handle: handle, slug_path: slug_path, **options) + end + + def plan_version_browse_path(plan, version, **options) + handle, slug_path = Urls::Canonical.split(plan.url_path) + browse_version_path(handle: handle, slug_path: slug_path, + revision: version.revision, **options) + end + + def plan_version_diff_browse_path(plan, version, **options) + handle, slug_path = Urls::Canonical.split(plan.url_path) + browse_version_diff_path(handle: handle, slug_path: slug_path, + revision: version.revision, **options) end # Level-view link for either kind of row, so folder and plan lists diff --git a/engine/app/helpers/coplan/plans_helper.rb b/engine/app/helpers/coplan/plans_helper.rb index 4efa08d1..29f929dd 100644 --- a/engine/app/helpers/coplan/plans_helper.rb +++ b/engine/app/helpers/coplan/plans_helper.rb @@ -10,7 +10,7 @@ module PlansHelper # # `folder` isn't in it: a folder names a place, so it travels as path # segments rather than a query param. See workspace_path. - WORKSPACE_LINK_PARAMS = %i[scope filter plan_type tag updated].freeze + WORKSPACE_LINK_PARAMS = %i[filter plan_type tag updated].freeze # A workspace URL for the library being browsed, carrying the current # filters with `overrides` applied; pass nil to clear one. diff --git a/engine/app/models/coplan/folder.rb b/engine/app/models/coplan/folder.rb index 8ee44607..60f83a6e 100644 --- a/engine/app/models/coplan/folder.rb +++ b/engine/app/models/coplan/folder.rb @@ -38,6 +38,11 @@ class Folder < ApplicationRecord # produces a NOT NULL-satisfying row. The method is idempotent. before_validation :assign_slug before_save :assign_slug + # Claiming a segment is the folder's half of a contest plans are also + # entering — see Library#lock_namespace!. Held from here through + # #disambiguate_shadowed_plans, so a plan created at this level while + # the folder lands can't be missed by the sweep *and* miss the folder. + before_save :lock_library_namespace # A rename or a move leaves behind one prefix alias, which covers # every plan and subfolder underneath — O(renames), not O(documents). before_save :stash_previous_url_path @@ -211,6 +216,14 @@ def assign_slug self.slug = CoPlan::Slug.call(name).presence || "folder" end + # Only when the folder is actually taking a segment: a description edit + # contests nothing and shouldn't queue behind anything. + def lock_library_namespace + return unless will_save_change_to_slug? || will_save_change_to_parent_id? + + library&.lock_namespace! + end + # Captured before the write, because afterwards `ancestors` walks the # *new* parent chain and the old path is no longer reconstructible. def stash_previous_url_path @@ -249,14 +262,19 @@ def disambiguate_shadowed_plans # Plans addressed at this folder's own level: those filed in its # parent, or the library's loose plans when it's a root folder. + # + # A locking read, in the same shape and for the same reasons as + # AssignSlug#siblings — see the note there. Without it, a plan that took + # this segment while we waited on the namespace lock stays invisible + # behind this transaction's snapshot and never gets moved aside. def shadowed_plans scope = if parent_id.present? - Plan.where(id: PlanPlacement.where(folder_id: parent_id).select(:plan_id)) + Plan.joins(:placement).where(coplan_plan_placements: { folder_id: parent_id }) else library.unfiled_plans end - scope.where(slug: slug, slug_suffix: nil) + scope.where(slug: slug, slug_suffix: nil).lock end def parent_cannot_create_cycle diff --git a/engine/app/models/coplan/home_feed.rb b/engine/app/models/coplan/home_feed.rb index 147886e3..e9e55357 100644 --- a/engine/app/models/coplan/home_feed.rb +++ b/engine/app/models/coplan/home_feed.rb @@ -24,9 +24,15 @@ def summary_parts end # Returns Items sorted by most recent activity, newest first. - def self.build(now: Time.current) + # + # `tag` narrows the whole feed to one tag. That's what a tag chip does: + # "everyone's work on this", which spans libraries and so belongs here + # rather than inside anybody's — a library is one person's shelf. + def self.build(now: Time.current, tag: nil) since = now - WINDOW - listed = Plan.publicly_listed.select(:id) + listed = Plan.publicly_listed + listed = listed.with_tag(tag) if tag.present? + listed = listed.select(:id) rollups = Hash.new do |h, k| h[k] = { published: false, edits: 0, comments: 0, last_at: nil } diff --git a/engine/app/models/coplan/library.rb b/engine/app/models/coplan/library.rb index e2048c5d..703eb099 100644 --- a/engine/app/models/coplan/library.rb +++ b/engine/app/models/coplan/library.rb @@ -127,6 +127,34 @@ def writable_by?(user) owner_type == "CoPlan::User" && owner_id == user.id end + # Takes the library's namespace lock: every URL segment under this + # handle is claimed one writer at a time. + # + # A segment is contested across *models* — a folder and a plan at the + # same level want the same word, and the folder wins (Urls::Resolve), + # which is why Folder#disambiguate_shadowed_plans moves the plan aside. + # No unique index can span coplan_folders and coplan_plans, and a plan's + # own scope is split across coplan_plans (the slug) and + # coplan_plan_placements (the level), so "one segment, one thing" is + # decided by reading before writing. This is what makes that read + # authoritative: check and claim happen with nobody else writing here. + # + # A library is one person's, so nothing ever waits on this in practice, + # and one lock per transaction — always the library being written to — + # means writers can't deadlock against each other. + # + # Row-level `FOR UPDATE` rather than an advisory lock, so it works the + # same on MySQL and PostgreSQL and releases itself on commit. + def lock_namespace! + unless self.class.connection.transaction_open? + raise "lock_namespace! must run inside a transaction — outside one " \ + "the lock is released immediately and guards nothing" + end + + self.class.lock.where(id: id).pick(:id) + self + end + private # A personal library takes its owner's username — their ldap — so the diff --git a/engine/app/services/coplan/plans/assign_slug.rb b/engine/app/services/coplan/plans/assign_slug.rb index 804d36e8..aa769101 100644 --- a/engine/app/services/coplan/plans/assign_slug.rb +++ b/engine/app/services/coplan/plans/assign_slug.rb @@ -38,7 +38,13 @@ def initialize(plan:, folder: :unset, previous_path: :unset, record_alias: true) @record_alias = record_alias end + # Runs inside the caller's transaction — a plan's own `before_save`, + # a move, or the folder rename that shadowed it — and takes the + # destination library's namespace lock before reading, so the + # `contested?` answer is still true when the caller writes it. def call + target_library&.lock_namespace! + @plan.slug = derive @plan.slug_suffix = contested? ? assign_suffix : nil @@ -48,6 +54,13 @@ def call private + # Where the plan is landing, which is the namespace it competes in. + # Mid-move that's the destination, not where it's leaving: the level + # it vacates can't gain a collision by losing a member. + def target_library + @target_library ||= @folder&.library || @plan.library + end + def default_previous_path @plan.slug.present? ? @plan.url_path : nil end @@ -96,54 +109,82 @@ def strip_leading(tokens, phrase) tokens end - # Something else at this level already holds the segment. Checked - # against the folder's placements, which is the plan's real - # uniqueness scope — see the note in AddPlanSlugsAndUrlAliases about - # why this isn't a DB constraint yet. + # Something else at this level already holds the segment. + # + # Enforced here rather than by a unique index, and not only because + # the slug and the location live in different tables: the contest + # crosses *models*. A folder and a plan compete for the same segment, + # and no single index spans coplan_folders and coplan_plans. So the + # write path is where "one segment, one thing" can actually be + # decided, and it decides it from the read below. + def contested? + taken.include?([ @plan.slug, nil ]) + end + + # Every segment already spoken for at this level, as [slug, suffix]. # # Sibling *folders* count too. Urls::Resolve hands the segment to a # folder when both want it — mistaking a folder for a plan would # break a whole subtree — so a plan sharing a folder's slug would - # have no reachable address at all. Folders never take a suffix; - # plans do, which makes the plan the one that moves. - def contested? - return true if sibling_folders.where(slug: @plan.slug).exists? - - siblings.where(slug: @plan.slug, slug_suffix: nil).exists? + # have no reachable address at all. Folders never take a suffix, which + # is what makes the plan the one that moves. + # + # Read with FOR UPDATE, and not because these rows are being written. + # Under MySQL's REPEATABLE READ a plain SELECT answers from the + # snapshot this transaction took at its *first* read, which happened + # before Library#lock_namespace! — so the writer we just finished + # waiting for would still be invisible and we would confidently claim + # the segment it had already taken. A locking read sees the latest + # committed row instead. Safe under the namespace lock: one writer per + # library is in here at a time, and neither query reaches outside the + # library it holds. + def taken + @taken ||= ( + sibling_folders.lock.pluck(:slug).map { |slug| [ slug, nil ] } + + siblings.lock.pluck(:slug, :slug_suffix) + ).to_set end # The folders that sit at the same level of the URL as this plan: the # children of the folder it's filed in, or the library's root folders # when it's filed nowhere. def sibling_folders - library = @folder&.library || @plan.library - return Folder.none if library.nil? + return Folder.none if target_library.nil? - library.folders.where(parent_id: @folder&.id) + target_library.folders.where(parent_id: @folder&.id) end + # The plans at that same level. + # + # Both shapes below keep coplan_plans in the *outer* query, and that's + # the load-bearing part. FOR UPDATE reads latest-committed only for + # what the query itself scans, so a plan reached through + # `where(id: )` — the shape this used to + # have — gets filtered out by the snapshot before the locking scan + # ever sees it, and the lock guards a set that was never refreshed. + # + # The filed case joins, because the placement row is what decides + # membership and so has to be read fresh too. The unfiled case can + # leave placements in a subquery: a plan created a moment ago has no + # placement in either the snapshot or the present, so `NOT IN` gives + # the same answer from both, and the plan row itself comes off the + # locking scan. def siblings scope = if @folder - Plan.where(id: @folder.placements.select(:plan_id)) + Plan.joins(:placement).where(coplan_plan_placements: { folder_id: @folder.id }) else - Plan.where(id: unfiled_sibling_ids) + target_library&.unfiled_plans || Plan.none end scope = scope.where.not(id: @plan.id) if @plan.persisted? scope end - # At a library root, a plan's siblings are the other plans its - # library shows there — the ones with no placement of their own. - def unfiled_sibling_ids - @plan.library&.unfiled_plans&.select(:id) || [] - end - # Keeps trying until the pair is free. Random rather than sequential # so the suffix says nothing about how many plans came before. def assign_suffix 10.times do candidate = SUFFIX_LENGTH.times.map { SUFFIX_ALPHABET[SecureRandom.random_number(SUFFIX_ALPHABET.length)] }.join - return candidate unless siblings.where(slug: @plan.slug, slug_suffix: candidate).exists? + return candidate unless taken.include?([ @plan.slug, candidate ]) end SecureRandom.hex(4) end diff --git a/engine/app/services/coplan/plans/place.rb b/engine/app/services/coplan/plans/place.rb index a1812981..609b9e1d 100644 --- a/engine/app/services/coplan/plans/place.rb +++ b/engine/app/services/coplan/plans/place.rb @@ -48,6 +48,38 @@ def call if @folder && @folder.library_id != @library.id return Result.new(error: "Folder belongs to a different library") end + # Filing requires the plan to be listable for you — an unlisted + # draft someone linked you can be read, but filing it into a + # browsable library would surface what its author hasn't published. + if @folder && !PlanPolicy.new(@actor, @plan).listed? + return Result.new(error: "Only published plans (or your own drafts) can be filed") + end + + ActiveRecord::Base.transaction { move! } + rescue ActiveRecord::RecordInvalid => e + Result.new(error: e.record.errors.full_messages.join(", ")) + rescue ActiveRecord::RecordNotUnique + # Two concurrent files of the same plan raced past the read; the + # unique plan_id index caught it. The transaction above rolled back, + # so retrying once starts a clean one — and the placement now + # exists, which makes this a plain move. + raise if @retried_unique + @retried_unique = true + @plan.reload_placement + retry + end + + private + + # One transaction, because a move is one fact: the row that says where + # the plan lives, the slug that follows from it, the alias that keeps + # the old address working, and the two audit trails either all happen + # or none do. The namespace lock is taken first and released by the + # commit, so nothing else claims a segment here while the new slug is + # being chosen — see Library#lock_namespace!. + def move! + @library.lock_namespace! + placement = @plan.placement old_path = placement&.folder&.path # Captured before the write: afterwards the plan resolves through @@ -64,13 +96,6 @@ def call return Result.new(placement: nil) end - # Filing requires the plan to be listable for you — an unlisted - # draft someone linked you can be read, but filing it into a - # browsable library would surface what its author hasn't published. - unless PlanPolicy.new(@actor, @plan).listed? - return Result.new(error: "Only published plans (or your own drafts) can be filed") - end - if placement return Result.new(placement:) if placement.folder_id == @folder.id @@ -84,20 +109,8 @@ def call reslug(old_url_path, @folder) log_move(old_path, @folder.path) Result.new(placement:) - rescue ActiveRecord::RecordInvalid => e - Result.new(error: e.record.errors.full_messages.join(", ")) - rescue ActiveRecord::RecordNotUnique - # Two concurrent files of the same plan raced past the read; the - # unique plan_id index caught it. Retry once — the placement now - # exists, so this becomes a plain move. - raise if @retried_unique - @retried_unique = true - @plan.reload_placement - retry end - private - # Filing is a move of the document, not curation of a personal # shelf, so it takes authority on both sides: write access to the # destination library (checked above) and a claim on the plan where diff --git a/engine/app/services/coplan/urls/canonical.rb b/engine/app/services/coplan/urls/canonical.rb index a08fdfde..95b0d5f6 100644 --- a/engine/app/services/coplan/urls/canonical.rb +++ b/engine/app/services/coplan/urls/canonical.rb @@ -14,15 +14,13 @@ module Urls # The absolute form needs a host, which only a request knows, so # `browse_url` stays in the helper. module Canonical - # `///`, or the id form for a plan whose - # slug hasn't been backfilled yet. Both have to work while slugs - # fill in, and `/plans/` 301s here once one exists. + # `///` — the plan's one and only address. + # There's no id form to fall back to: `slug` is NOT NULL and a plan + # lives in exactly one library, so this is total. def self.plan_path(plan, **options) - routes = CoPlan::Engine.routes.url_helpers handle, slug_path = split(plan.url_path) - return routes.plan_path(plan, **options) if slug_path.blank? - - routes.browse_path(handle: handle, slug_path: slug_path, **options) + CoPlan::Engine.routes.url_helpers + .browse_path(handle: handle, slug_path: slug_path, **options) end # Splits "handle/rest/of/path" into its two route segments. A path diff --git a/engine/app/views/coplan/agent_instructions/show.text.erb b/engine/app/views/coplan/agent_instructions/show.text.erb index 0f02c44d..0d5dde8a 100644 --- a/engine/app/views/coplan/agent_instructions/show.text.erb +++ b/engine/app/views/coplan/agent_instructions/show.text.erb @@ -77,7 +77,9 @@ Optional query params: `?visibility=draft` (your own private/unshared plans) or "<%= @base %>/api/v1/plans/$PLAN_ID" | jq . ``` -Returns: `id`, `title`, `visibility`, `archived`, `current_content` (markdown), `current_revision`, `references` (array of linked resources). +Returns: `id`, `title`, `url`, `visibility`, `archived`, `current_content` (markdown), `current_revision`, `references` (array of linked resources). + +`url` is the document's address, and there is exactly one — `<%= @base %>///`, reading like the document it points at. **When you tell a human about a plan, give them the `url`, never the id.** It follows a retitle or a move on its own, and the old address keeps resolving. ### Get Plan Snapshot (Recommended) @@ -232,7 +234,7 @@ Each folder includes `id`, `name`, `library_id`, `parent_id`, `path` (e.g. `"Tea - `folder_path` finds or creates the whole hierarchy in your library — the easiest way to organize. - Alternatively pass `folder_id` with an existing folder's ID from your library. - Pass an empty `folder_id` (`{"folder_id": ""}`) to take a plan off your shelf. -- `folder_id`/`folder_path` in plan responses are **yours**: where *you* shelved that plan, `null` if you haven't. +- `folder_id`/`folder_path` in plan responses say where the plan lives — one answer, the same for everybody, `null` for a plan nobody has filed yet. Filing a plan that's already filed **moves** it; there's no second copy. **Guidelines:** - Check `GET <%= @base %>/api/v1/library` (your library's map — folder tree with descriptions and counts) before creating new folders — reuse the existing structure. diff --git a/engine/app/views/coplan/home/show.html.erb b/engine/app/views/coplan/home/show.html.erb index b8a343b1..25732e05 100644 --- a/engine/app/views/coplan/home/show.html.erb +++ b/engine/app/views/coplan/home/show.html.erb @@ -1,14 +1,30 @@ -<% content_for(:title, "Feed — CoPlan") %> +<% content_for(:title, @tag ? "##{@tag} — Feed — CoPlan" : "Feed — CoPlan") %>

Feed

+ <%# A tag chip narrows the feed rather than opening a list of its own — + same clearable-state treatment the workspace filters get. %> + <% if @tag %> +
+ + #<%= @tag %> + <%= link_to "✕", home_path, class: "workspace__filter-clear", + aria: { label: "Clear ##{@tag} filter" } %> + +
+ <% end %>
<% if @items.empty? %>
-

No published activity in the last two weeks.

-

When someone publishes, edits, or discusses a plan, it shows up here.

+ <% if @tag %> +

No published activity tagged #<%= @tag %> in the last two weeks.

+

<%= link_to "See everything →", home_path, class: "landing__inline-link" %>

+ <% else %> +

No published activity in the last two weeks.

+

When someone publishes, edits, or discusses a plan, it shows up here.

+ <% end %>
<% else %> <% @items_by_date.each do |date, items| %> @@ -44,7 +60,7 @@
by <%= profile_link(plan.created_by_user) %> <% plan.tags.first(3).each do |tag| %> - <%= link_to "##{tag.name}", plans_path(scope: "all", tag: tag.name), class: "home__item-tag" %> + <%= link_to "##{tag.name}", home_path(tag: tag.name), class: "home__item-tag" %> <% end %>
diff --git a/engine/app/views/coplan/plan_versions/diff.html.erb b/engine/app/views/coplan/plan_versions/diff.html.erb index 858fd796..7516c4c1 100644 --- a/engine/app/views/coplan/plan_versions/diff.html.erb +++ b/engine/app/views/coplan/plan_versions/diff.html.erb @@ -13,7 +13,7 @@ · <%= @version.change_summary %> <% end %> - <%= link_to "View full version →", plan_version_path(@plan, @version), class: "text-sm", data: { turbo_frame: "_top" } %> + <%= link_to "View full version →", plan_version_browse_path(@plan, @version), class: "text-sm", data: { turbo_frame: "_top" } %> <% if @diff %> diff --git a/engine/app/views/coplan/plan_versions/show.html.erb b/engine/app/views/coplan/plan_versions/show.html.erb index ffdf52ee..2a829ac7 100644 --- a/engine/app/views/coplan/plan_versions/show.html.erb +++ b/engine/app/views/coplan/plan_versions/show.html.erb @@ -1,4 +1,4 @@ -

<%= link_to "← Back to history", history_plan_path(@plan) %>

+

<%= link_to "← Back to history", plan_history_browse_path(@plan) %>

diff --git a/engine/app/views/coplan/plans/_history.html.erb b/engine/app/views/coplan/plans/_history.html.erb index 4aad0316..2e8e5067 100644 --- a/engine/app/views/coplan/plans/_history.html.erb +++ b/engine/app/views/coplan/plans/_history.html.erb @@ -15,7 +15,7 @@
- +

Loading…

diff --git a/engine/app/views/coplan/plans/_toolbar.html.erb b/engine/app/views/coplan/plans/_toolbar.html.erb index c22bb512..01d8c944 100644 --- a/engine/app/views/coplan/plans/_toolbar.html.erb +++ b/engine/app/views/coplan/plans/_toolbar.html.erb @@ -16,7 +16,7 @@ <% can_edit = allowed_to?(plan, :edit_content?) %>