pal-e-docs

Review: Catalog tab: app portfolio with App Store links

review-1820-2026-07-17 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Replaces scope of #11
  • [x] Repo — ldraney/intelligentstaffingsystems
  • [x] User Story — As a lead or client, browse portfolio
  • [x] Context — Portfolio positioning, target audience described
  • [x] File Targets — 6 files listed (modify or create)
  • [x] Feature Flag — None (correct, core tab)
  • [x] Acceptance Criteria — 7 items
  • [x] Test Expectations — 6 tests + run command
  • [x] Constraints — Turbo Native, responsive, ISS tokens, read-only
  • [x] Checklist — PR/tests/unrelated
  • [x] Related — project-iss, #11

Traceability

  • [x] story:catalog label — Epic 3 (US-3.1–3.2), Lead/Client, "Browse portfolio by three pillars; admin CRUD"
  • [x] story note verified — found in project-iss user-stories section
  • [x] arch:rails label — present on board item
  • [ ] arch note MISSING — [SCOPE] No arch-rails note exists in pal-e-docs. Create architecture note arch-rails for the Rails application component.
  • [x] Forgejo issue — ldraney/intelligentstaffingsystems#50, open

File Targets

  • [x] app/controllers/catalog_controller.rb — verified: EXISTS with index + show actions, all roles see same content
  • [x] app/views/catalog/index.html.erb — verified: EXISTS with card grid, pillar grouping, and tech tags
  • [ ] app/views/catalog/_catalog_entry.html.erb — ISSUE: Does not exist. Index renders cards inline. Creating this partial is a refactoring extraction — acceptable as a "create" target but should be noted.
  • [x] app/views/catalog/show.html.erb — verified: EXISTS with breadcrumb, gallery, tech sidebar
  • [x] app/assets/stylesheets/catalog.css — verified: EXISTS
  • [x] app/models/catalog_entry.rb — verified: EXISTS but does NOT have app_store_url field. Schema confirms no such column.
  • [ ] Migration file — ISSUE: [BODY] Issue says "add app_store_url field if not present" but no migration file is listed in File Targets. Add db/migrate/XXXXXX_add_app_store_url_to_catalog_entries.rb to the File Targets section.

Repo Placement

OK — issue filed on ldraney/intelligentstaffingsystems, all file targets are in that repo. No cross-repo concerns.

Dependencies

  • #1791 (#11) — original catalog ticket (decomposed, sprint:4). Its children #38 (model), #39 (browsing UI), #41 (admin CRUD) are all DONE. The catalog infrastructure this ticket builds on is fully shipped.
  • #1829 (#49) — tab bar restructure (sprint:A, validation). Catalog tab already wired in navigation helper. No blocker.
  • No blocking items in in_progress or todo columns.
  • No downstream items depend on this ticket.
  • Dependencies are not explicitly documented in the issue body, but the Lineage section references #11 which covers the relationship.

Acceptance Criteria

7 AC items. Assessment of each:
  • AC 1 (portfolio description) — NEW work. Current header says "What We Build" / "Browse our catalog." Needs portfolio-focused language about customization and target audience. Testable via assert_select.
  • AC 2 (target audience copy) — NEW work. Testable via assert_select for content.
  • AC 3 (cards with App Store link) — PARTIALLY DONE. Cards already show title, description, screenshot, pillar. App Store link is the new addition. Testable.
  • AC 4 (external browser for App Store links) — NEW work. Requires data-turbo="false". This is the first use of this pattern anywhere in the app. Testable via assert_select for the data attribute.
  • AC 5 (responsive card grid) — ALREADY DONE. Grid exists in catalog.css.
  • AC 6 (all three roles see identical content) — ALREADY DONE. Tests exist and pass.
  • AC 7 (detail page with expanded info, multiple screenshots, tech stack) — ALREADY DONE. Show page exists with gallery and tech sidebar.
Concern: An implementing agent may not realize that ACs 5-7 are already satisfied, wasting time rebuilding existing functionality. The issue should clarify which ACs are "verify existing" vs "implement new."

Blast Radius

  • No data-turbo="false" or target="_blank" patterns exist anywhere in the app currently. This ticket introduces the first external-link-in-Turbo-Native convention. The pattern should be documented or noted as precedent-setting for future tickets.
  • The landing page uses a constant ApplicationHelper::APP_STORE_URL for a single TestFlight link. Catalog entries will have per-entry app_store_url columns — a different mechanism. No conflict.
  • Admin catalog CRUD (app/controllers/admin/catalog_controller.rb) exists and would need updating to allow editing the new app_store_url field, but the issue correctly says "Files NOT to touch: app/controllers/admin/catalog_controller.rb — admin CRUD is separate." This is consistent — a follow-up ticket should handle the admin form update.

Decomposition Assessment

6 file targets in 1 repo. 7 AC items (over the 5-item guideline), but 3 are already implemented. Real new work: 1 migration, update header copy, add App Store link buttons with data-turbo="false", add model validation, optionally extract a partial. Estimated agent work: ~4 minutes. No decomposition needed.

Recommendation

  • [BODY] Add migration file to File Targets: db/migrate/XXXXXX_add_app_store_url_to_catalog_entries.rb — adds app_store_url:string column.
  • [BODY] Clarify which ACs are "verify existing" vs "implement new." ACs 5-7 are already satisfied by Sprint 4 work. Mark them as verification-only or remove them to avoid confusing the implementing agent.
  • [SCOPE] Create architecture note arch-rails for the Rails application component. This label is used across many board items.