Update docs for multi-user portfolio vision #20
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/vision-and-architecture"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
README.md: Rewritten as intro + TOC, updated tech stack with Keycloak, added visibility.md to TOCdocs/architecture.md: Added Keycloak auth flow, user roles (superadmin/member/anonymous), new ER diagram (users, clicks, feature_flags tables), feature flags designdocs/infrastructure.md: Added auth infrastructure section, OIDC flow, Keycloak secrets, custom domain placeholderdocs/user-stories.md: Added US-2 click tracking, US-3 feature flags dashboard, US-4 role-based visibility, US-8 authentication, US-9 contact channeldocs/visibility.md: New file defining three access tiers with mermaid diagrams and schema approachTest Plan
Review Checklist
Related Notes
project-palinks— project pagearch-palinks— architecture notePR #20 Review
DOMAIN REVIEW
Tech stack: Documentation only (Markdown, Mermaid diagrams). No code changes. Reviewed for internal consistency, Mermaid syntax correctness, information accuracy, and cross-file coherence.
Critical inconsistency -- role taxonomy diverges across files.
The PR introduces two incompatible role/visibility models that directly contradict each other:
docs/architecture.md(User Roles table)docs/architecture.md(ER diagram:users.role)docs/architecture.md(ER diagram:links.visibility)docs/visibility.md(Access Tiers)docs/visibility.md(Schema + Ruby scope)docs/user-stories.md(US-4 acceptance criteria)These are fundamentally different designs. Architecture.md describes a 4-tier model (public < lead < collaborator < admin). Visibility.md describes a 3-tier model (public < member < superadmin). The ER diagram enum values don't match the visibility.md schema. The Ruby
visible_toscope in visibility.md usessuperadmin/memberbut architecture.md's data model says the column holdspublic, lead, collaborator, admin.A developer implementing from these docs would get contradictory instructions depending on which file they read first.
Mermaid diagrams: Syntax is valid across all files. The flowcharts, sequence diagrams, ER diagram, journey map, and state diagrams all use correct Mermaid syntax and should render on Forgejo.
No sensitive information leaked: No secrets, credentials, or internal IPs exposed. Keycloak secret names are listed as environment variable names only (appropriate for docs). The Tailscale domain is already public via funnel.
BLOCKERS
1. Role taxonomy inconsistency across files (BLOCKER)
As detailed above,
architecture.mdanduser-stories.mddefine 4 roles (admin, collaborator, lead, public) whilevisibility.mddefines 3 tiers (superadmin, member, public). The ER diagram, Ruby scope, and acceptance criteria all conflict. This must be reconciled before merge -- whichever model is correct, all files must agree.2. "Closes #15" is incorrect (BLOCKER)
The PR body says
Closes #15 -- domain spike referenced in infrastructure docs. Issue #15 is "Spike: Route palinks.app domain to production" and is still open. This PR does NOT resolve that spike --docs/infrastructure.mdline 169 explicitly says the routing approach is "under investigation" and links to Spike #15 as future work. Merging this PR would auto-close an unresolved spike. Change toReferences #15orRelated to #15.3.
docs/visibility.mdmissing from README TOC (BLOCKER)The PR adds a new file
docs/visibility.mdbut does not add it to the Docs section inREADME.md. The README lists architecture, infrastructure, pipeline, user-stories, and rails-notes -- but not visibility. A new doc file that is not discoverable from the README is an oversight that should be fixed in this PR.NITS
Architecture.md Auth Model text says "two tiers of users" but the section immediately below defines four roles. The introductory sentence should match whichever role count is decided.
User story renumbering gap: US-1 through US-10 are defined, but the old US-3 (Annotate and Organize) lost its Mermaid flowchart in the renumbering to US-6. The old diagram was removed but no replacement was added. This is minor but worth noting -- the other user stories retained their diagrams.
Architecture.md Auth Model flowchart shows "click tracking" in the anonymous path (
Anonymous experience / click tracking, public links only) but click tracking is described as a feature flag elsewhere. If click tracking is behind a feature flag, anonymous users might not get it by default. The diagram implies it is always-on for anonymous users.visibility.md references
palinks.appas the public URL, but the README and other docs still referencepalinks.tail5b443a.ts.netas the live URL. The custom domain is noted as "Planned" in infrastructure.md. Usingpalinks.appin visibility.md prose is premature until the domain spike (#15) is resolved.Open Questions section in visibility.md is good practice for a docs PR but the first question ("Should public visitors see anything before logging in") seems already answered by the architecture docs and US-1 which describe anonymous users seeing public links. Consider reconciling or removing.
SOP COMPLIANCE
docs/vision-and-architecture. Does not follow{issue-number}-{kebab-case-purpose}convention. If this PR is meant to close an issue, the branch should be named accordingly (e.g.,15-docs-multi-user-vision). However, since "Closes #15" is incorrect anyway, this may just be an unlinked docs PR -- in which case, what issue does it belong to?PROCESS OBSERVATIONS
VERDICT: NOT APPROVED
Three blockers must be resolved:
docs/visibility.mdto the README Docs TOCPR #20 Re-Review
PREVIOUS BLOCKER RESOLUTION
All three blockers from the prior review have been addressed:
Role taxonomy inconsistency -- RESOLVED. The Keycloak role names are now consistent across all five files:
superadminandmemberas Keycloak roles,public/anonymous as the unauthenticated tier. architecture.md line 76 explicitly clarifies: "Public is the absence of auth, not a Keycloak role. Superadmin and member are the two Keycloak roles." visibility.md mirrors this framing. The ER diagram, the visibility scope, and the role table all align."Closes #15" would auto-close an unresolved spike -- RESOLVED. The PR body now reads "Related to #15, #16, #17, #18 -- spikes informed by these docs" which correctly references without triggering auto-close. Issue #15 remains open as expected.
visibility.md missing from README TOC -- RESOLVED. README.md line 15 now includes
- [Visibility](docs/visibility.md) -- access tiers and role-based link filtering, placed logically between Infrastructure and User Stories.DOMAIN REVIEW
This is a docs-only PR (Markdown and Mermaid diagrams). No code changes. Domain checks applied: documentation quality, internal consistency, cross-reference accuracy.
Role terminology -- minor residual inconsistency (nit, not blocker):
user-stories.mdUS-1 line 36: "leads see lead links" -- "lead" is not a defined role. Per the architecture, leads would bemembertier users. This is an acceptance criterion phrasing, not a role definition, so it reads more as an audience description than a taxonomy violation.user-stories.mdUS-9 line 170: "Contact option visible to lead and collaborator roles" -- references "lead and collaborator roles" which are audience segments, not Keycloak roles. Per the architecture there are only two Keycloak roles (superadmin, member).user-stories.mdUS-3 flowchart line 89: "Admin?" decision node -- uses "Admin" rather than "superadmin". This is a diagram display label and reads naturally in context.These are all in user-stories.md where audience-oriented language is appropriate. The authoritative role definitions in architecture.md and visibility.md are precise and consistent. Not a blocker.
Cross-references verified:
Mermaid diagrams: 14 diagrams across 4 files. Syntax is correct. Entity relationships in the ER diagram match the visibility scope logic in visibility.md. Auth flow diagrams in architecture.md and infrastructure.md are consistent (OIDC redirect flow, token exchange, role assignment).
BLOCKERS
None.
NITS
US-1 acceptance criteria (user-stories.md line 36): "leads see lead links" could say "members see member + public links" to match the defined taxonomy. Low priority -- the intent is clear.
US-9 acceptance criteria (user-stories.md line 170): "visible to lead and collaborator roles" could say "visible to authenticated users (member role)" for precision. The current wording implies more Keycloak roles than exist.
US-3 flowchart (user-stories.md line 89): "Admin?" decision node could say "Superadmin?" to match the role name used everywhere else. Cosmetic only.
Auth Model section (architecture.md line 48): "The auth flow supports two tiers of users" -- technically three tiers (superadmin, member, anonymous). The sentence seems to mean "two tiers of authenticated users" but could be clearer.
SOP COMPLIANCE
docs/vision-and-architecture-- docs-only, no parent issue number expected)project-palinks,arch-palinks)PROCESS OBSERVATIONS
This PR establishes the architectural north star for palinks' evolution from personal bookmarks to multi-user portfolio. The four open spikes (#15-#18) are correctly referenced as "Related" rather than closed. The docs provide enough structure to guide implementation without over-specifying -- the TODO placeholders (link inventory audit in visibility.md, contact channel specifics in US-9) are appropriately deferred to their respective spikes.
VERDICT: APPROVED