pal-e-docs

Review: Tab bar restructure to Catalog / Projects / Communications / Account + CRM (re-review)

review-1829-2026-07-17-r2 Review

review ready
Project

Verdict: READY

Re-review of board item #1829. Previous review (review-1829-2026-07-17) returned NEEDS_REFINEMENT. All five findings have been addressed in the updated issue body.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Standalone, supersedes #10
  • [x] Repo -- ldraney/intelligentstaffingsystems
  • [x] User Story -- present (As a logged-in user / tab bar matching role / navigate main sections)
  • [x] Context -- present (old vs new tab structure, role visibility rules)
  • [x] File Targets -- present, complete (5 modify targets + 3 test files + 6 blast-radius views)
  • [x] Feature Flag -- none, structural change (appropriate)
  • [x] Acceptance Criteria -- 7 items
  • [x] Test Expectations -- 4 items + run command
  • [x] Constraints -- present (fixed: now references application.css :root, not deleted docs/ui-ux.md)
  • [x] Checklist -- present
  • [x] Related -- present (project-iss, #10)

Traceability

  • [x] story:navigation label -- "Bottom tab bar with role-correct tabs" (Epic 2 + role-tab matrix, All roles)
  • [x] story note verified -- found in project-iss user-stories section
  • [x] arch:frontend label -- present on board item
  • [x] arch note verified -- arch-frontend-iss note exists in pal-e-docs (status: active, project: iss)
  • [x] Forgejo issue -- #49, open

File Targets

Modify targets (all verified):
  • [x] app/views/layouts/application.html.erb -- verified: renders tab bar partial
  • [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 at line 14 with old structure (catalog, messages, profile, admin)
  • [x] app/assets/stylesheets/tab_bar.css -- verified: exists, uses design tokens
  • [x] config/routes.rb -- verified: no routes for /projects, /communications, /account, /crm (404s expected per issue)
Test files (all verified):
  • [x] test/controllers/tab_bar_test.rb -- verified: hardcodes old tab names (Messages, Profile, Admin) and counts (3 for lead/client, 4 for admin)
  • [x] test/helpers/navigation_helper_test.rb -- verified: asserts TAB_ROLES keys [:catalog, :messages, :profile, :admin] at line 34; asserts per-role visible tabs with old keys
  • [x] test/controllers/messages_controller_test.rb -- verified: line 58 asserts "Messages" tab as active, line 61 asserts .tab-bar-tab.is-active label "Messages"
Blast-radius views (all verified):
  • [x] app/views/messages/index.html.erb -- verified: content_for(:current_tab, "messages") at line 2
  • [x] app/views/messages/inbox.html.erb -- verified: content_for(:current_tab, "messages") at line 2
  • [x] app/views/admin/catalog/index.html.erb -- verified: content_for(:current_tab, "admin") at line 2
  • [x] app/views/admin/catalog/show.html.erb -- verified: content_for(:current_tab, "admin") at line 2
  • [x] app/views/admin/catalog/new.html.erb -- verified: content_for(:current_tab, "admin") at line 2
  • [x] app/views/admin/catalog/edit.html.erb -- verified: content_for(:current_tab, "admin") at line 2
Non-affected views (confirmed safe):
  • [x] app/views/catalog/index.html.erb -- sets content_for(:current_tab, "catalog"), key unchanged, no action needed
  • [x] app/views/catalog/show.html.erb -- sets content_for(:current_tab, "catalog"), key unchanged, no action needed

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. No conflict.
  • #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:
  • 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
  • Old tabs removed -- testable by asserting absence of Messages, Profile, Admin labels
Route strategy clarified: no new routes needed. Unimplemented tabs 404 until their feature tickets ship, matching the existing pattern documented in navigation_helper.rb comments (lines 6-9).

Blast Radius

  • content_for(:current_tab) renames: 6 view files documented in issue with specific key changes (messages->communications, admin->crm). 2 catalog views use "catalog" key which is unchanged.
  • Test suite: 3 test files documented in issue, all hardcode old tab structure.
  • docs/user-stories.md: Still references old tab names. Not in scope for this ticket -- follow-up only.

Decomposition Assessment

5 modify targets + 3 test files + 6 blast-radius views = 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.

Previous Review Findings -- All Resolved

  • [x] [BODY] Missing test file targets -- now listed under "Test files to update" with specific assertions to change
  • [x] [BODY] Missing blast-radius views -- now listed under "Blast-radius views" with specific content_for renames
  • [x] [BODY] Route contradiction -- resolved: issue now states "no new routes needed; 404s expected"
  • [x] [BODY] Stale constraint reference -- fixed: now references app/assets/stylesheets/application.css :root
  • [x] [SCOPE] arch-frontend note missing -- arch-frontend-iss created (active, project: iss)

Recommendation

No action needed. Scope is solid, all file targets verified, traceability complete, route strategy clarified, and the ticket fits in a single agent pass.