pal-e-docs

Review: Tab bar restructure to Catalog / Projects / Communications / Account + CRM

review-1829-2026-07-17 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Standalone, supersedes #10
  • [x] Repo -- ldraney/intelligentstaffingsystems
  • [x] User Story -- present
  • [x] Context -- present
  • [x] File Targets -- present (but incomplete, see below)
  • [x] Feature Flag -- none, structural change
  • [x] Acceptance Criteria -- 7 items
  • [x] Test Expectations -- 4 items + run command
  • [x] Constraints -- present (but stale reference)
  • [x] Checklist -- present
  • [x] Related -- present

Traceability

  • [x] story:navigation label -- "Bottom tab bar with role-correct tabs"
  • [x] story note verified -- found in project-iss user-stories section (Epic 2 + role-tab matrix, All roles)
  • [x] arch:frontend label -- present on board item
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-frontend for the frontend component
  • [x] Forgejo issue -- #49, open

File Targets

Listed targets (all verified):
  • [x] app/views/layouts/application.html.erb -- verified: renders tab bar partial at line 27
  • [x] app/views/shared/_tab_bar.html.erb -- verified: tab bar partial with role logic via visible_tabs
  • [x] app/helpers/navigation_helper.rb -- verified: TAB_ROLES constant at line 14 with old structure (catalog, messages, profile, admin)
  • [x] app/assets/stylesheets/tab_bar.css -- verified: exists, uses design tokens from application.css
  • [x] config/routes.rb -- verified: exists, but NO routes for /projects, /communications, /account, /crm
Missing from file targets (blast radius):
  • [ ] test/controllers/tab_bar_test.rb -- ISSUE: hardcodes old tab names (Messages, Profile, Admin) and counts (3 for lead/client, 4 for admin). Will break.
  • [ ] test/helpers/navigation_helper_test.rb -- ISSUE: asserts TAB_ROLES keys are [:catalog, :messages, :profile, :admin] (line 34). Asserts lead/client see [:catalog, :messages, :profile]. Asserts tab_icon for old keys. Will break.
  • [ ] test/controllers/messages_controller_test.rb -- ISSUE: line 58 references "lead sees Messages tab as active". Will break if tab key changes.
  • [ ] app/views/messages/index.html.erb -- ISSUE: sets content_for(:current_tab, "messages"), needs update to "communications"
  • [ ] app/views/messages/inbox.html.erb -- ISSUE: sets content_for(:current_tab, "messages"), needs update to "communications"
  • [ ] app/views/admin/catalog/index.html.erb -- ISSUE: sets content_for(:current_tab, "admin"), needs update to "crm"
  • [ ] app/views/admin/catalog/show.html.erb -- ISSUE: sets content_for(:current_tab, "admin"), needs update to "crm"
  • [ ] app/views/admin/catalog/new.html.erb -- ISSUE: sets content_for(:current_tab, "admin"), needs update to "crm"
  • [ ] app/views/admin/catalog/edit.html.erb -- ISSUE: sets content_for(:current_tab, "admin"), needs update to "crm"

Repo Placement

OK -- issue filed on ldraney/intelligentstaffingsystems, all file targets are in that repo. Single-repo change.

Dependencies

  • #10 (Bottom tab bar navigation) -- done, superseded by this ticket. OK.
  • #42 (Turbo Streams live delivery) -- in todo, messaging related, not blocking this ticket.
  • No blocking dependencies found on the board.

Acceptance Criteria

7 AC items, all testable in principle:
  • Tab counts per role -- testable via assert_select on rendered HTML
  • Active tab indicator -- testable via assert_select for active CSS class
  • Tab icons and labels -- testable via assert_select for .tab-bar__icon and .tab-bar__label
  • TAB_ROLES enforcement -- testable via unit test on NavigationHelper
  • Turbo Native WebView -- testable by checking data-turbo-action attribute (no full page reload)
  • Old tabs removed -- testable by asserting absence of Messages, Profile, Admin labels
Gap: No AC specifies what happens when a user clicks a tab whose controller hasn't shipped yet (Projects, Account, CRM). The existing codebase comment says 404s are expected, but the issue says "ensure routes exist for all tab landing pages" -- these contradict.

Blast Radius

  • content_for(:current_tab) renames: 8 view files set current_tab with old keys (messages, admin). Renaming tab keys without updating these views means no tab will highlight as active on those pages.
  • Test suite: 3 test files hardcode old tab structure -- all will fail after this change.
  • docs/user-stories.md: Still references old tab names (Catalog, Messages, Profile). Not critical for this ticket but should be noted for follow-up.

Decomposition Assessment

5 explicit file targets + ~9 blast radius files = ~14 files, all in 1 repo, all the same conceptual change (tab rename/restructure). 7 AC items. Estimated agent work: 3-5 minutes. No decomposition needed.

Recommendation

  • [SCOPE] Create architecture note arch-frontend for the frontend component in pal-e-docs.
  • [BODY] Add missing file targets for tests: test/controllers/tab_bar_test.rb, test/helpers/navigation_helper_test.rb, test/controllers/messages_controller_test.rb (line 58).
  • [BODY] Add blast-radius view files to file targets: app/views/messages/index.html.erb, app/views/messages/inbox.html.erb, app/views/admin/catalog/{index,show,new,edit}.html.erb -- all need content_for(:current_tab) key renames.
  • [BODY] Clarify route strategy: should placeholder routes be added for /projects, /communications, /account, /crm, or are 404s expected? Current routes.rb has none of these.
  • [BODY] Fix stale constraint reference: docs/ui-ux.md has been deleted. Update to reference app/assets/stylesheets/application.css :root for design tokens.