From 6b9c2ef0d8e6b31770899f2d402d708b92610f4b Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 24 Aug 2026 10:52:45 -0500 Subject: [PATCH 1/4] One address per plan: the readable URL is the only URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /_/plans was still the page I actually read my own work on, which is the tell that the browsable scheme hadn't finished landing. A document that has two addresses has one people paste and one people see, and the id form kept winning because it was the one every internal link produced. So the id form stops existing as a destination. - Every plan has a slug. BackfillPlanSlugs gives one to every plan that predates the scheme and then makes `slug` NOT NULL, so "no readable address yet" is no longer a state the app has to have an answer for. The rules are inlined in the migration, like the folder and handle backfills before it: a migration has to keep producing the same URLs years after the app's rules move on. - The id fallback dies. plan_browse_path, Urls::Canonical.plan_path and plan_browse_url always return the readable path, and rel=canonical always renders rather than only when a slug happened to exist. - A document's pages hang off the document. /hampton/liveorder/cart-roadmap gains /edit, /history, /history/2 and /history/2/diff, so trimming the tail walks back to the thing being edited — the same property every other prefix in this scheme has. Versions are addressed by the revision number already on screen, not by id. When the tail turns out not to hang off a document (/sam/notes/history, where "notes" is a folder) the path is put back together and resolved as a place, so a plan someone titled "History" keeps its address. - /_/plans is gone. /plans and /plans/ stay in the frozen legacy block as 301s, so every old link converges on the readable form — query string and all — instead of rendering a second copy of the page. The id-based `_` scope keeps only the mutations, which don't need readable URLs because a button doesn't. - "All plans" was a scope toggle on a page that no longer exists; the answer to "where is someone else's work" is now their library, so the specs that exercised scope=all visit /theirhandle instead. - The API says where a plan is: plan_json carries `url`, and the agent instructions mention it, so an agent that creates a plan can hand back the address a human will actually use. Co-Authored-By: Claude Opus 5 --- ...60823193530_backfill_plan_slugs.co_plan.rb | 243 ++++++++++++++++++ db/schema.rb | 4 +- .../coplan/api/v1/base_controller.rb | 8 + .../coplan/api/v1/plans_controller.rb | 4 + .../controllers/coplan/browse_controller.rb | 63 ++++- .../controllers/coplan/folders_controller.rb | 8 +- .../app/controllers/coplan/home_controller.rb | 11 +- .../coplan/notifications_controller.rb | 2 +- .../coplan/plan_versions_controller.rb | 33 --- .../controllers/coplan/plans_controller.rb | 101 ++++---- .../controllers/coplan/welcome_controller.rb | 9 +- engine/app/helpers/coplan/browse_helper.rb | 49 +++- engine/app/helpers/coplan/plans_helper.rb | 2 +- engine/app/models/coplan/home_feed.rb | 10 +- engine/app/services/coplan/urls/canonical.rb | 12 +- .../coplan/agent_instructions/show.text.erb | 6 +- engine/app/views/coplan/home/show.html.erb | 24 +- .../views/coplan/plan_versions/diff.html.erb | 2 +- .../views/coplan/plan_versions/show.html.erb | 2 +- .../app/views/coplan/plans/_history.html.erb | 2 +- .../app/views/coplan/plans/_toolbar.html.erb | 4 +- .../views/coplan/plans/_version_item.html.erb | 2 +- engine/app/views/coplan/plans/index.html.erb | 7 +- engine/app/views/coplan/plans/show.html.erb | 10 +- .../coplan/welcome/_default_landing.html.erb | 4 +- engine/config/routes.rb | 49 +++- .../20260823000000_backfill_plan_slugs.rb | 242 +++++++++++++++++ .../analytics_instrumentation_spec.rb | 6 +- spec/requests/browse_spec.rb | 94 ++++++- spec/requests/notifications_spec.rb | 2 +- spec/requests/plan_content_editing_spec.rb | 4 +- spec/requests/plan_versions_spec.rb | 6 +- spec/requests/plans_spec.rb | 229 ++++++++--------- spec/requests/sessions_spec.rb | 10 +- spec/requests/welcome_spec.rb | 17 +- spec/support/browse_path_helpers.rb | 65 ++++- spec/system/checkbox_spec.rb | 22 +- spec/system/comment_anchoring_spec.rb | 2 +- spec/system/comment_ux_spec.rb | 92 +++---- spec/system/deck_ux_spec.rb | 16 +- spec/system/folders_workspace_spec.rb | 44 ++-- spec/system/history_spec.rb | 16 +- spec/system/human_editing_spec.rb | 14 +- spec/system/mermaid_anchor_spec.rb | 4 +- spec/system/nav_stimulus_spec.rb | 6 +- spec/system/references_spec.rb | 18 +- spec/system/voice_commenting_spec.rb | 2 +- 47 files changed, 1143 insertions(+), 439 deletions(-) create mode 100644 db/migrate/20260823193530_backfill_plan_slugs.co_plan.rb delete mode 100644 engine/app/controllers/coplan/plan_versions_controller.rb create mode 100644 engine/db/migrate/20260823000000_backfill_plan_slugs.rb 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..8ae1a4f3 --- /dev/null +++ b/db/migrate/20260823193530_backfill_plan_slugs.co_plan.rb @@ -0,0 +1,243 @@ +# 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? + # Only an un-suffixed slug blocks the segment; a suffixed one has + # already moved out of the way. + (taken[key] ||= Set.new) << plan["slug"] if plan["slug_suffix"].blank? + 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..91d9ef6b 100644 --- a/engine/app/controllers/coplan/api/v1/plans_controller.rb +++ b/engine/app/controllers/coplan/api/v1/plans_controller.rb @@ -419,6 +419,10 @@ def plan_json(plan) { 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..31bbd617 100644 --- a/engine/app/controllers/coplan/browse_controller.rb +++ b/engine/app/controllers/coplan/browse_controller.rb @@ -5,20 +5,42 @@ 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 + params.delete(:page) + result = resolve(slug_path: [ params[:slug_path], *page_tail ].compact_blank.join("/")) + end return head :not_found unless result.found? # A stale-but-recognizable path: 301 so the address bar, and @@ -32,11 +54,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 +89,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/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/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?) %>