pal-e-docs

Review: Live DM messaging: real-time client-admin threads

review-1825-2026-07-18 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type: Feature
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:messaging label — Epic 4 (US-4.1–4.2), verified in project-iss user-stories section
  • [x] story note verified — found in project-iss user-stories table (key: messaging, role: Lead/Client/Admin, metric: "One thread per lead; admin sees all")
  • [ ] arch:rails label — arch note MISSING — [SCOPE] Create architecture note arch-rails (note: "rails" is the whole app framework; this may be intentionally broad and not warrant a dedicated arch note)
  • [x] Forgejo issue — #55, open

File Targets

  • [x] app/controllers/messages_controller.rb — verified: EXISTS, fully implemented (thread view, admin inbox, create, role-based access, Turbo Stream response)
  • [x] app/views/messages/index.html.erb — verified: EXISTS
  • [x] app/views/messages/_message.html.erb — verified: EXISTS
  • [x] app/views/messages/_form.html.erb — verified: EXISTS
  • [x] app/models/message.rb — verified: EXISTS (lead_id FK, sender_sub, body, sent_at; broadcasts_to :lead; scopes)
  • [x] app/assets/stylesheets/messages.css — verified: EXISTS
  • [ ] app/javascript/controllers/scroll_controller.js — ISSUE: Does not exist at this path. Functionality implemented at app/javascript/controllers/messages_thread_controller.js (auto-scroll + broadcast alignment)
  • [x] config/cable.yml — verified: EXISTS (solid_cable in production, async in dev)
  • [x] db/migrate/XXX_create_messages.rb — verified: EXISTS as 20260705000003_create_messages.rb

Repo Placement

OK — issue filed on ldraney/intelligentstaffingsystems, all targets within same repo.

Dependencies

  • #12 "Messaging model and conversation threads" (8pts, sprint:4, decomposed) — parent scope, in backlog
  • #40 "Message CRUD (decomp:1792)" — DONE, implements message controller, views, model
  • #42 "Turbo Streams live delivery + cable fix (decomp:1792)" — DONE, implements real-time delivery + Action Cable
  • #52 Communications tab (sprint:B) — DONE, the DM button linking to messages exists
  • #54 (story:communications, sprint:C) — in backlog, may relate
Critical finding: Tickets #40 and #42 (both completed) already implement 95%+ of what #55 describes. The feature is live in the codebase with comprehensive tests (35 controller tests + 14 model tests covering all listed test expectations).

Acceptance Criteria

  • [x] Message model: lead_id, sender_sub, body, sent_at — DONE (migration + model exist)
  • [x] One thread per client (lead_id is the thread) — DONE
  • [x] Client can view their own thread and send messages — DONE + tested
  • [x] Admin can view and reply in any client's thread — DONE + tested (inbox + thread view)
  • [ ] Lead role blocked from DM endpoint — CONTRADICTS docs/messaging.md which explicitly states: "The controller does not distinguish between the two roles — UI-level gating on the Communications tab prevents leads from reaching the messages route." Existing tests verify leads CAN access messages.
  • [x] Real-time delivery via Turbo Streams — DONE + tested
  • [x] Solid Cable adapter configured — DONE (production uses solid_cable)
  • [x] Message bubbles: sent right (navy), received left (bordered) — DONE + tested
  • [x] Auto-scroll to latest message — DONE (messages_thread_controller.js)
  • [x] sender_sub set from session — DONE + tested
9 of 10 AC are met. The 1 unmet AC (lead-blocking) contradicts the established architecture in docs/messaging.md.

Blast Radius

Messaging feature is self-contained. Communications controller (not-to-touch) exists separately. Turbo Streams pattern is consistent with existing code. No sibling services affected. If lead-blocking were added at the controller level, it would break existing tests that verify lead access.

Decomposition Assessment

10 acceptance criteria exceeds the 5-AC threshold. However, 9/10 AC are already implemented and tested. The actual remaining work is trivial (either close as done, or resolve the lead-blocking decision). No decomposition needed — the ticket needs SCOPE REDUCTION, not decomposition.

Recommendations

  • [BODY] The feature is 95%+ implemented by completed tickets #40 and #42. Either close this issue as already done, or rewrite scope to describe ONLY the delta (the lead-blocking decision).
  • [BODY] Fix file path: app/javascript/controllers/scroll_controller.js → app/javascript/controllers/messages_thread_controller.js (already exists with full scroll + alignment functionality).
  • [BODY] AC "Lead role blocked from DM endpoint" contradicts docs/messaging.md which documents UI-only gating as the intentional architecture. Either update the AC to match the doc, or update the doc if controller-level blocking is actually desired.
  • [SCOPE] Decide whether lead-blocking should be controller-level (contradicts existing doc + tests) or remain UI-only (current implementation). This is a human decision.
  • [SCOPE] arch:rails has no backing note in pal-e-docs. Consider whether a broad "rails" arch note is needed or if the label is acceptable without one.