pal-e-docs

Review v2: Hours tab: clock in/out/break UI

review-1938-2026-07-27-v2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item #1938 — Hours tab: clock in/out/break UI (Forgejo issue #333, 5 points, sprint:7)
Re-review after refinement. Previous review: review-1938-2026-07-27. Of the 5 issues flagged, 4 were addressed correctly. One remains.

Previous Issues — Status

  • [x] arch:rails label — Verified. convention-architecture-ids lists arch:rails under Application as "Rails application (models, controllers, views)". General category label, not project-specific. Acceptable.
  • [x] hours_controller.rb annotation — Landed. Issue body now reads app/controllers/hours_controller.rb (created by #332).
  • [x] Stimulus timer test expectation — Landed. Test expectations note server-derived correctness: timer calculates elapsed from started_at minus break durations. JS is pure display.
  • [x] Decomposition rejection — Landed. Context section justifies keeping 10 AC as one ticket: all items on single page, tightly coupled. Splitting nav/page-shell from timer-widget would deliver an empty page.
  • [ ] User story on project page — NOT LANDED. project-landscaping-assistant user-stories section has 15 entries; "crew-time-tracking" is not among them.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Related to #331 (parent), depends on #332 (model ticket)
  • [x] Repo — ldraney/landscaping-assistant
  • [x] User Story — As a crew member or lead, clock in/out/break
  • [x] Context — Present, includes decomposition justification
  • [x] File Targets — 2 create, 5 modify, exclusions listed
  • [x] Feature Flag — time_tracking (registered by #332)
  • [x] Acceptance Criteria — 10 items
  • [x] Test Expectations — 6 items + run command
  • [x] Constraints — 4 items
  • [x] Checklist — standard
  • [x] Related — project + parent + story
All template sections present and complete.

Traceability

  • [x] story:crew-time-tracking label — present on board item
  • [ ] story note MISSING — [SCOPE] No "crew-time-tracking" entry in project-landscaping-assistant user-stories section. Must be added before work begins.
  • [x] arch:rails label — present on board item. Verified against convention-architecture-ids: general category label "Rails application (models, controllers, views)" under Application section. Not project-specific.
  • [x] arch note verified — arch-rails note exists in pal-e-docs. Describes Rails architecture generally; label is a cross-project category per convention.
  • [x] Forgejo issue — #333, open, valid URL

File Targets

  • [x] app/views/hours/index.html.erb — TO CREATE. Directory does not exist; expected for greenfield.
  • [x] app/javascript/controllers/timer_controller.js — TO CREATE. Controllers directory exists with 19 Stimulus controllers. Uses eagerLoadControllersFrom so no index.js changes needed.
  • [x] app/views/layouts/application.html.erb — verified exists. Nav uses 5-slot grid (nav-slot-1 through nav-slot-5). Slot 2 is currently Week (admin-only). Hours would reuse slot 2 for member/lead since Week and Hours are mutually exclusive by role.
  • [x] app/controllers/application_controller.rb — verified exists. TAB_ROLES at line 132 has: today, week, profile, properties, crew. Adding :hours for member/lead is straightforward.
  • [x] app/controllers/hours_controller.rb (created by #332) — correctly annotated as created by dependency #332. File does not exist yet, as expected.
  • [x] app/assets/stylesheets/application.css — verified exists.
  • [x] config/routes.rb — verified exists. No hours routes present; ticket correctly notes "may already be added by #332."

Repo Placement

Correct. All work is in ldraney/landscaping-assistant. Forgejo issue filed on the same repo. No cross-repo scope.

Dependencies

  • #332 (TimeEntry data model + API + flag) — sprint:6, backlog. Hard dependency. Creates the controller, models, routes, and feature flag that #333 consumes. Sprint ordering (6 before 7) is correct. Dependency documented in Lineage section.
  • #331 (Time Tracking: Hours Tab) — parent umbrella, backlog. No blocking dependency.
  • #334 (Hours tab: daily/weekly summary) — sprint:7, backlog. Sibling ticket. Independent — can run in parallel with #333.
  • No items in in_progress or needs_approval block this ticket. #1940 (Keycloak redirect bug) is in_progress but unrelated.

Acceptance Criteria

10 acceptance criteria. Testability assessment:
  • AC 1-2 (tab visibility by role): Testable via request specs. Covered by test expectations.
  • AC 3 (Clock In creates entry): Testable via request spec. Covered.
  • AC 4-6 (Timer display, break pause/resume): Client-side JS only. Addressed by test expectation: timer correctness is server-derived (elapsed calculated from started_at minus break durations). JS is pure display. Request spec for clock-in verifies timer data in response.
  • AC 7 (Clock Out stops timer): Mix of server + client. Server side covered.
  • AC 8 (Today's log shows entries): Testable via request spec content check.
  • AC 9 (Timer survives page refresh): Server-derived reconstruction verified by clock-in response test.
  • AC 10 (Tab hidden when flag disabled): Covered by test expectations.
All criteria have reasonable test coverage strategy.

Blast Radius

No existing time tracking code in the codebase (grep confirmed: no matches for time_tracking, time_entry, TimeEntry, clock_in, clock_out). Clean greenfield addition. Nav slot 2 reuse (Week for admin, Hours for member/lead) is architecturally sound — roles are mutually exclusive. No downstream consumers affected.

Decomposition Assessment

  • File targets: 7 (2 create + 5 modify), all in 1 repo — OK
  • Acceptance criteria: 10 (threshold: >5) — EXCEEDS
  • Estimated agent work: Borderline 5 minutes.
Decomposition waived. Justification in issue body is sound: all 10 AC describe states of one cohesive widget on one page. Splitting nav/page-shell from timer-widget would deliver an empty page as the first ticket — artificial boundary. The AC are tightly coupled (cannot test break without clock-in, cannot test clock-out without timer). Accepted.

Recommendation

  • [SCOPE] Create user story entry "crew-time-tracking" on project-landscaping-assistant user-stories section. This was flagged in v1 review and claimed as addressed, but the entry is still missing. Row should include: Key=crew-time-tracking, Role=Crew member/Lead, Summary=Clock in/out and track breaks from Hours tab, Success Metric=Timer runs with server-derived accuracy, entries logged with worked time.