feat(help): searchable in-page help for the planning page - #1703
Merged
Conversation
The planning page is not admin-gated: any user holding time_planning_plugin_access can edit almost everything on it, including the 2001-line day-cell editor, with nothing on the page explaining flex, pauses, netto hours, or what saving triggers. The 67 existing matTooltips are icon labels, not explanations, and no help affordance of any kind exists in either repo. Design: one help registry defines ~36 explainable things; four surfaces read from it (info icon, side panel, two tours, inline hints), so an answer written once appears everywhere. Prose lives in its own per-locale files so the 25 existing i18n files stay untouched, with per-entry fallback to English. No backend, no new dependency, no change to eform-angular-frontend. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
"Admin" means Microting, not a customer role, so help copy must not mention administrator capabilities, explain what someone with more access could do, or account for why a control did nothing. The worker-column entry was written the wrong way round: it explained that clicking opens worker settings for administrators. It now names only the four things the column displays — worker, weekly hours, tags, and the status icons for the rules applying to them. Adds a Copy rules section carrying this plus the two authoring rules already exercised elsewhere in the spec: name which value is meant when a label is reused, and describe what the screen shows rather than how the server computes it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
A registry of control descriptions is not searchable in the way users actually ask. Nobody searches for a control named "vacation" — no such control exists — so "how do I register vacation for a worker" would return nothing. Adds a second entry kind. Control entries answer "what is this?" and are anchored on screen; task entries answer "how do I do X?", carry ordered steps and a related list pointing back at the controls they touch. Twelve tasks, taking the registry to ~48 entries. Search sits at the top of the panel: substring, case- and diacritic-insensitive so Danish folds, run over both the active locale and the English fallback, with tasks ranked above controls and the task list shown instead of an empty result. Per-locale keywords carry the synonyms — ferie, sygdom, afspadsering, barsel appear in no English string. Records the flag rule the three leave-related tasks exist to explain: day flags render as checkboxes but are mutually exclusive, and ticking one rewrites netto hours — 0 for DayOff and VacationDayOff, the planned hours for every other flag. Adjacent checkboxes, opposite results. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Eleven tasks, TDD throughout, with the plugin cycle's dual-subagent pre-commit gate built into every commit step. Two reviewers ran against the first draft. Their findings, all fixed: The local test loop did not work at all. Jest runs from the frontend repo and its testMatch is scoped to that rootDir, so specs written only in the plugin repo were never discovered — every "run it and confirm it fails" step would have reported "No tests found". Task 0 now establishes the --roots invocation and the node_modules symlink it needs, both verified end to end before this plan was finalised. The plan also contradicted its own constraint by adding UI labels to the 25 shared locale files; those are now HelpUiStrings in help/i18n/, with tests asserting no help template uses the translate pipe. The dialog tour could never start, since mounting the component only subscribes. The panel's "replay the tour" affordance was specified but never built. HelpTourComponent marked a tour seen at mount — state$ replays null to new subscribers — which would have suppressed the automatic first run for every first-time user; recording moved into the service, guarded so a tour that could not start is not marked seen. Smaller fixes: dayCell.save and the three flex entries were never anchored; the "tasks rank above controls" test was vacuous when a query returned only tasks; the Danish banned-word regex missed administratoren and administratorens; three tests asserted prose the plan never mandates. Also corrects the spec: cdkConnectedOverlayUsePopover does not exist in CDK 20.2.14, several line citations were off by one, and the .help-text pattern is used twice in this plugin, not ~8 times. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
… injecting the tour service Pre-flight scan found Task 7 (panel) injecting HelpTourService, which Task 8 creates — it would not have compiled in task order. The panel now raises replayTourRequested and Task 9 binds it, which also keeps the panel independent of the tour. Task 5's openInPanel output was produced and never consumed, leaving the popover's More in help link inert. Task 9 now binds it, with a wiring test covering both bindings. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Adds the content foundation for the planning-page help system: the id union and entry/prose types, a 48-entry registry with anchors, tour steps and task cross-references, and complete English prose written from the actual toolbar, grid and day-dialog templates. The day-type entries carry the rule the UI hides: the flags render as checkboxes but are mutually exclusive, and ticking one rewrites the day's netto hours -- Day off and Vacation day off to zero, every other type to that day's planned hours. An integrity spec enforces completeness, unique ids and tour steps, resolvable related ids, and that no user-facing copy mentions administrators. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
task.registerDayOff explained only that Day off and Vacation day off set the day to zero hours, and never said that ticking one unticks whatever day type was set before. The other two leave tasks both say so. Reuse their phrasing so all three carry both halves of the rule. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
All 48 registry entries in Danish, plus the help chrome labels, registered as the 'da' locale. Day-type copy names the labels the checkboxes actually render: Fridag and Afspadsering zero the day, while Ferie and the look-alike Ferie fridag keep the planned hours. Also covers the per-entry English fallback in HelpContentService, which was unreachable by test until a second locale existed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
I had written that Vacation and Vacation day off "sit next to each other and produce opposite results". They do not: in render order Vacation is 2nd and Vacation day off is 10th, with Time off, Maternity and the two children's-sick-day types between them. The confusion that actually bites is name versus behaviour. Time off keeps the day's planned hours while the similarly-named Day off and Vacation day off zero them. Danish is worse: the shipped labels are Fridag (zero), Afspadsering (zero) and Ferie fridag — which keeps the hours despite being named a fridag. Found by the Task 3 implementer while translating, and verified against TimePlanningMessagesEnum and both shipped label maps. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The English day-type entry claimed Vacation and Vacation day off sit next to each other. In render order Vacation is 2nd and Vacation day off is 10th, so the claim is false; the Danish carried the same framing. Adjacency was never the trap anyway. Time off (da: Ferie fridag) is named like the two types that zero the day but behaves like the ones that keep the planned hours. Both locales now say that plainly, in the day-type entry, the day-off task and the netto override entry. day-type-copy.spec.ts walks HELP_LOCALES so neither the false claim nor a missing warning can come back in any locale. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The warning half of day-type-copy.spec.ts checked that 'Time off', 'Day off', 'Vacation day off', /planned/i and /zero hours/i each appeared somewhere in the entry. The ordinary type-by-type description names every type and both outcomes anyway, so all five held with the warning sentence deleted - the exact regression the test was meant to catch. It now requires the look-alike type's name, a despite-the-name construction, both zeroing types and both outcomes in the SAME sentence. The bare co-occurrence checks stay, under an honest name. Both locales' dayCell.flags move onto the despite-the-name construction that task.registerDayOff and dayCell.nettoOverride already used, so all three sites phrase the trap the same way. Mutation-checked: deleting only the warning sentence from enUS.ts turns that one test red and leaves the other six green. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…eywords Tests 3 and 4 were vacuous: 'læge' and 'laege' don't exist in help content, so both queries returned the task fallback list. Replaced with real keywords that trigger fold() and verified via mutation testing (empty LIGATURES map breaks the tests, confirming non-vacuity). - Test 3 now searches 'fravær'/'fraVAER' (æ → ae folding) - Test 4 now searches 'løn'/'lon' (ø → o folding) + å assertion unchanged - Both assert non-empty results to block content edits that remove keywords Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…rådt' The original å assertion used 'årstid' which does not appear in help content, so both queries returned the tasksOnly fallback. Replaced with 'fratrådt' (a keyword on toolbar.showResigned) and verified via mutation testing (disabling the combining-mark strip in fold() breaks the å assertion). - 'fratrådt' folds to 'fratradt' (å → a via NFD + combining-mark strip) - Both accented and folded queries now assert non-empty results - Equality assertion guards against removing the keyword from content Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The spec illustrated the search test as "a Danish query folds diacritics (laege finds læge)". Neither term appears anywhere in the help content, so both queries miss and fall through to the same fallback list — the example asserts nothing, and the plan's tests inherited the flaw. Corrected to a term that actually appears in the content, and to require a non-empty result, since two queries that both match nothing compare equal whether or not folding works. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Adds the in-page help info button: a mat-icon-button that opens a small connected-overlay popover with the entry's title and short text, plus a "More in help" affordance that emits openInPanel for a later task to bind. Uses a plain cdkConnectedOverlay - cdkConnectedOverlayUsePopover does not exist in the installed @angular/cdk 20.2.14. This still stacks above an open MatDialog because CDK appends later overlays after the dialog pane in .cdk-overlay-container. Chrome labels come from HelpContentService.ui(), never the ngx-translate pipe, so the plugin's 25 shared locale files gain no keys. An unregistered helpId renders nothing rather than throwing: the prose getter returns undefined when the registry misses, and the template body sits behind *ngIf. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…a note Two post-review corrections to tp-help-icon. Drop the backdrop. A transparent full-page backdrop swallows the first click, and on this page every day cell is a click target that opens the day editor, so dismissing a popover cost a planner an extra click. Switch to hasBackdrop=false with (overlayOutsideClick), which dismisses on the same click that reaches the control underneath. Escape handling is unchanged. Correct the ARIA role. The popover carried role="dialog" while implementing no focus management, advertising modal semantics it does not honour. Use role="note", which describes what it is: a small non-modal informational aside. No focus management added on purpose - a non-modal popover that steals focus would be worse here. aria-expanded on the trigger is unchanged. Adds three tests: outside-click dismissal with no backdrop present, inside-click keeping it open so "More in help" stays clickable, and the role/aria-label pair. Both new assertions were confirmed to fail against the previous template. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The spec said the popover is dismissed by a backdrop click, but its actual requirement is dismissability. A transparent full-page backdrop swallows the first click, and on this page every day cell is a click target that opens the day editor — so a planner dismissing a popover would click twice to reach the cell underneath. overlayOutsideClick gives the same dismissal without the tax. Raised independently by the implementer and the reviewer. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…e trigger The component bound no scroll strategy, so it inherited CDK_CONNECTED_OVERLAY_SCROLL_STRATEGY's default, which is reposition. That kept the popover glued to its trigger: in the horizontally scrolling grid it could end up floating over unrelated columns, or trailing a trigger the planner had already scrolled past. The design spec promised dismissal on scroll, so make the code match the promise. Build the strategy explicitly with Overlay.scrollStrategies.close() and bind it via [cdkConnectedOverlayScrollStrategy]. Nothing now depends on the injected default. The dismissal set matches the spec: Escape, outside click, scroll. Two tests. One asserts the built strategy is a CloseScrollStrategy. The other is behavioural: it opens the popover, dispatches a scroll event on the document, and asserts the popover leaves the DOM - exercising the real chain from ScrollDispatcher through the close strategy to the (detach) binding, with nothing stubbed. Both were confirmed to fail when the binding is removed and when the strategy is swapped for reposition. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Implement Task 6 of the planning help system: a reusable inline hint component that renders entry short text with tone-based styling (info/warn). Follows TDD with all 3 component tests passing, maintaining 53 pre-existing tests green. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
Add test to verify the warn-tone icon swap from 'info' to 'warning' is rendered. Prevents regression in the ternary that controls the icon text content, which is a critical visual signal in the day editor. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
HelpPanelService holds the panel's open/close state and optional deep-link target, so a help icon anywhere on the page can open the panel without owning it. HelpPanelComponent browses the registry grouped by section and switches to ranked search results as soon as a query is typed. The panel does not depend on the tour: it closes itself and raises replayTourRequested for the host to wire up. All chrome labels come from HelpUiStrings, never the shared ngx-translate catalogue. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…face token A search that matched nothing rendered a bare count above twelve tasks, which read as twelve hits. HelpSearchResult now carries an additive fallback flag, set where the task list is produced, and the panel names the query alongside the noResults line instead of a count. The kind badge carried an aria-hidden icon and no accessible name, so the task/control distinction was absent for screen readers. HelpUiStrings gains kindTask, kindControl and resultCount — this feature's own locale files, not the plugin's shared catalogue — and the badge and count now use them. The panel and the help popover painted var(--bg), the page ground, which is a tint rather than a surface on the workspace theme. Both now use --tp-td-bg. Also: Escape closes the panel, and a late-arriving isAdmin rebuilds the browse sections and the live results instead of leaving them stale. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
HelpTourService sequences the page and dialog tours from the registry, dropping steps whose data-tp-help anchor is not in the DOM rather than treating them as an error: the payroll-export control only renders for Microting staff, and the worker filter only when more than one site exists. "Seen" is recorded by the service - in emit() when a tour reaches its end and in stop() when it is skipped - never by the component. state$ is a BehaviorSubject seeded null, so a component that marked seen on a null state would do so at mount, permanently suppressing the automatic first run for every genuine first-time user. Both writes are guarded on steps.length > 0 so a tour that could not start is offered again. tp-help-tour renders the current step in a CDK connected overlay anchored to the element being explained, wrapping the querySelector result in an ElementRef as cdkConnectedOverlayOrigin requires. Skip/Next labels come from HelpUiStrings, never the ngx-translate pipe. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…chor goes The service is a singleton but Task 9 mounts tp-help-tour twice - once on the page, once in the day-cell dialog - so an instance now ignores state belonging to the other tour. Without this both cards rendered at once. Dropped the unread @input() isAdmin entirely: admin filtering happens in HelpContentService, via the opts passed to start(). Anchors were only validated at start(). If the current step's anchor left the DOM afterwards - the dialog closes mid-tour, a filter hides the worker select - the overlay closed but the tour stayed "running", leaving no card and no Skip. ngDoCheck now ends the tour when its anchor is gone (and re-points the overlay when the node was merely replaced), and emit() drops later steps whose anchors vanished, keeping the step total honest. Completed the role="dialog" contract: aria-modal="false", aria-labelledby on a per-instance title id, Escape via (overlayKeydown), and focus moved to Next when a card appears - the overlay sits at the end of <body>, out of tab reach. No focus trap; the tour is non-modal and must not lock the page it explains. Simplified stop() to `if (this.current)`: emit() already clears current, so the steps.length conjunct was unreachable and no mutation could hold it to account. The load-bearing guard stays in emit(). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The tour component never read isAdmin — admin filtering happens inside
HelpTourService.start(tour, { isAdmin }), which the container and the
dialog each call. The input was dead and has been removed, so Task 9's
templates must not bind it.
Task 8's review also found the tour input itself was dead: the component
mirrored whatever tour the singleton service was running, so Task 9's two
mounts would have rendered the same card simultaneously. Each instance
now renders only its own tour.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…he user does Splits the two endings. stop() is the user saying they are done - Skip or Escape - and still records the tour as seen, as does running to the last step. abort() ends the tour without recording it, and the anchor-loss path in ngDoCheck now uses it. An anchor vanishing is the page changing underneath the tour; it says nothing about what the user wanted. This matters most for the dialog tour, which starts the instant the day-cell dialog opens: a planner who opens a row, glances and closes it may have seen one step of six, and would otherwise never be offered it automatically again. Closing a dialog means "done with this row", not "done learning". Both directions are held to account: an anchor-loss end leaves the tour unseen and still offerable, while a user skip on that same tour records it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…og tour stop() marks a tour seen and is the user-initiated end, used by Skip, Escape and completion. abort() ends a tour without marking it seen, for the page changing underneath the tour. Someone who opens a day cell, glances and closes it has not seen the dialog tour and must still be offered it. Calling stop() from the dialog's teardown would reintroduce by hand the behaviour just removed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…page Mounts the help surfaces built in Tasks 5-8 on the pages they explain: - Toolbar `?` button, labelled from HelpUiStrings, plus data-tp-help anchors on all nine toolbar controls and the seven grid ones. - tp-help-panel and the page tour on the plannings container; the day-cell tour, thirteen tp-help-icons, every dayCell/flex anchor and the future-date warn hint in the workday dialog. - Both tours are started from their host, once, after the DOM they point at has rendered. The dialog ends its tour with abort(), not stop(), so glancing at a row and closing it does not count as having seen the tour. Three fixes the wiring exposed: - The tour scrolls each step's anchor into view. CDK's reposition strategy otherwise pushes the card on screen pointing at nothing, which is what steps 5 and 6 do as soon as a tenant has a screenful of workers. - The panel scrolls a deep-link target into view; it expanded the target but left it below the fold. - The panel now stacks above .cdk-overlay-container. The day-cell dialog is modal, so at z-index 900 its "More in help" link opened the panel behind the backdrop and read as a control that did nothing. mtx-grid renders its own header row and MtxGridColumn has no per-column header template, so grid.sortName is stamped from applyGridHelpAnchors() instead of a template. .main-content is marked cdkScrollable: it is a second overflow container inside mat-dialog-content, and ScrollDispatcher only sees containers carrying the directive, so popovers would not have dismissed on its scroll. help-wiring.spec.ts pins the contract: an anchor for every entry with a tourStep, every helpId and every data-tp-help resolving in the registry, both tours started from a component, no translate pipe in help templates and no new shared locale key. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…ner, gate the tour Review fixes for the help wiring. Layout: the five day-cell fields that gained a help icon were direct children of the dialog's flex column, where a flex item stretches. Rowing them up with their icon shrank them to mat-form-field's intrinsic width while the fields beside them stayed full width. They now use .field-with-help, which restores the stretch. Deliberately a new class, not a change to .flex-row: that one is also used by the per-shift time rows, where stretching the field would eat the space its reset, GPS and snapshot buttons sit in. Accessibility: z-index 1001 fixed stacking but not reachability. CDK's Dialog marks body siblings of .cdk-overlay-container aria-hidden while a modal is open, so the panel opened from a day-cell help icon was invisible to screen readers. The panel host is now moved into the overlay container while open and put back when it closes, and focus is handed to its search input. z-index is back to 900; pointer-events: auto is needed because the container opts out. Tab cycling still wraps inside the dialog's focus trap — a proper fix needs the TemplatePortal refactor and its own trap, which the existing panel spec (not modifiable here) is written against. Tour: HelpTourService records a tour as seen the moment it runs out of steps, in localStorage. Offering the page tour on an empty grid dropped steps 4-6 and suppressed them permanently. startPageTourOnce() now waits for rows. Tests: four assertions that could barely fail are replaced. The output-binding check compares counts instead of finding one binding; the header-stamp check asserts the call site rather than the import; the translate-key check freezes the templates' whole literal key set and scans all 26 locale files for the help chrome wording; the two panel-scroll cases are merged so both directions can fail. Each new assertion was verified by mutating the behaviour it covers. grid-help-anchors now takes the anchor from the registry rather than repeating the literal. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
.cdk-overlay-container is position:fixed with a z-index, so it is a stacking context, and CDK's prebuilt stylesheet puts .cdk-overlay-backdrop, .cdk-global-overlay-wrapper and .cdk-overlay-pane at z-index 1000 inside it. The panel at 900 therefore painted below all of them: opened from a day-cell help icon it sat dimmed under the dialog's backdrop, and because paint order decides hit-testing, a click meant for the panel landed on the backdrop and closed the dialog. pointer-events cannot rescue an element painted underneath. Raised to 1001 — scoped to the container, not to the page — and the comment claiming DOM order was enough is corrected. The new test resolves the intent rather than matching text: it reads the backdrop, wrapper and pane z-indexes out of @angular/cdk/overlay-prebuilt.css and requires the panel's declared value to exceed them. At 900 it fails. Also: - restoreFromOverlayContainer uses insertBefore against the sibling captured at move time, so the host stops walking past <tp-help-tour> on each cycle. - applyGridHelpAnchors warns once in dev when the registry has no anchor for grid.sortName instead of returning silently. - The panel takes focus only on a real closed -> open transition, and hands it back to the still-connected element that opened it. Writing that test found a second defect: buildSections() ran on every emission, handing *ngFor a fresh array and re-creating every entry node, so a second "More in help" destroyed focus and scroll position whoever held them. It is now confined to the same transition; only isAdmin changes what is listed, and ngOnChanges covers that. Each change was verified by mutation: four mutations, four failures, one per assertion. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…ch gaps Consolidated fix wave from the final whole-branch review. Both onboarding tours auto-start on a fresh profile, which is exactly what every Playwright context is, so every e2e shard would have run with a tour card over the grid rows and over the shift-1 fields and failed Playwright's actionability check. Keep the auto-start and seed the flag: a storage-state file naming tp.planning.tour.v1 for the config's own baseURL, wired as use.storageState, copied into the host by both workflows, and guarded by a jest test that imports TOUR_STORAGE_KEY rather than repeating it. Escape with the help panel open closed the day-cell dialog and discarded unsaved edits: the panel is a plain element, so CDK's OverlayKeyboardDispatcher reached MatDialogRef first. Listen on document in the capture phase and stop the event there. Also: harden the cross-locale search test so removing HELP_FALLBACK fails it; build the two inline hints the spec named but nobody wrote (the 24-hour validation and the empty grid) in both locales, and move the worker-column hint off the bottom of the table; render `related` as links that move the panel to the control; let the panel name the surface it was opened from, so the dialog tour is replayable and "Take the tour" inside the dialog no longer starts the page tour; drive the three help-icon tests through their bindings instead of their handlers; cover ui() locale resolution and the ngOnChanges false branch; end the page tour on openDay; and fold the repeated `ui`/`prose` getters into an abstract chrome base. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
HelpPanelService.open() defaults the surface to 'page', and openRelated() passed no surface. Opening help from inside the day-cell dialog and then following a related-control link therefore reset the surface, and "Take the tour" went back to replaying the page tour, whose anchors all sit behind the dialog backdrop. Navigating within the panel is not moving to another surface, so carry it through. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…ker column Help is admin-only for now: no ? button, no panel, no tour, no icon, no inline hint for anyone else. Gated centrally rather than at the eighteen call sites spread across three templates. HelpVisibilityService selects selectCurrentUserIsAdmin — the selector this plugin family standardises on — and is read by three choke points: HelpEntryChromeBase.prose (which both the icon and the hint template hang off, so one getter hides all 14 icons and all 3 hints), HelpTourService .start() (the single entry for both auto-start tours and the panel's replay button) and HelpPanelComponent (so a stray open() cannot render, reparent or steal focus). Only the ? button is gated in its own template, since it is plain toolbar markup with no help component behind it. The service subscribes live and deliberately not with take(1): the admin flag can arrive after the page is constructed, and a one-shot read that lands first would latch false and hide help from a real admin for the session. A tour refused by the gate is not marked seen. The gate is temporary, and a planner who was never offered the tour has not declined it. Second change: the grid.nameColumn hint above the grid was a blue banner, open from page load with no trigger and no dismiss, permanently occupying the top of the planning page. It is now a compact tp-help-icon in the same spot — click to open, click away to dismiss, like every other help affordance. MtxGridColumn still has no per-column header template, so it stays above the grid rather than inside the header cell. The other three hints are already conditional and are left alone. Existing help specs gained a two-line HelpVisibilityService stub instead of a mock store; no assertion was weakened, and the stub defaults to admin so the existing coverage still describes the admin case. Eleven new tests cover the gate, each with an admin half so the gate is distinguishable from the feature being deleted; all nine mutants of the gate were confirmed to go red. 184 help tests pass across 14 suites (173 before). ng build (AOT) clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
…ertion
Review follow-up.
The ? button was gated on the container's own isAdmin, which is a take(1)
read. The risk runs the opposite way from what I assumed when I wrote it:
loadAuthState dispatches synchronously at bootstrap, so the flag is there
before the lazy planning route is built, and the reachable failure is a later
flip TO non-admin — the cross-tab storage listener, a role change. The latch
then fails open and leaves a visible ? button whose panel refuses to render.
It now binds helpVisibility.isVisible$, the same live source the rest of the
help chrome is gated on; the day-cell dialog already subscribes live to that
selector, so the container was the outlier, not the idiom. payroll-export is
left alone — same latch, pre-existing, separate change.
Same cause in startPageTourOnce: pageTourOffered was set before start(), so a
tour the gate refused burned the once-per-page offer. The gate is now part of
the guard, before the latch, rather than being inferred from isRunning
afterwards — the container spec mocks start() to a no-op, so an isRunning
check would have failed an existing test for reasons unrelated to the gate.
Three test fixes, all assertions that could not fail:
- The not.toContain('<tp-help-hint helpId="grid.nameColumn"') guard could not
see the banner it named, which was written with a class attribute between
the two. Deleted rather than repaired: the exhaustive hintIds equality three
lines above catches a re-added hint however it is written, and a regex
beside it would be a second name for one fact. Its intent moved into the
comment.
- 'still offers a refused tour once help becomes visible' passed with the gate
deleted — an ungated start leaves hasSeen false too. Asserts isRunning now.
- toContain('tp-help-icon') also matched the tp-help-icon--name-column class,
so a hint wearing that class satisfied it. Tightened to '<tp-help-icon'.
Each of the three was confirmed to survive its own mutant before the fix and
go red after. Also hides the empty icon host for non-admins, so the reserved
margin above the grid goes with it.
184 help tests pass across 14 suites; 206 with the container spec. ng build
(AOT) clean — rerun because the ? button gained a real template binding.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
The whole help system is gated to admins for now — the ? button, the panel, both tours, every info icon and every inline hint. This is a staged rollout, not a change of audience: the content is still written for the team lead and the copy rules still bind, because customers see it when the gate lifts. Records where the gate lives, why a refused tour is not marked seen, and the two things to revisit when the gate is lifted. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01B5LtfrpnXpu2p9FrWFsSnB
There was a problem hiding this comment.
🟡 Changes recommended
CdkScrollable is added to imports in the Angular module, but it isn’t an NgModule/standalone import—ScrollingModule should be used or the build will fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new in-page help system for the time-planning “planning” page, centered around a registry-driven help catalogue that powers popovers, a searchable side panel, and two guided tours, with initial admin-only visibility gating.
Changes:
- Introduces a help registry + per-locale help content (EN/DA) plus services for content resolution, search (diacritic folding + fallback), panel state, and tours (seen-state in localStorage).
- Wires help affordances into the planning toolbar, grid, and the day-cell dialog via
data-tp-helpanchors, popover icons, inline hints, and auto-start tours. - Updates Playwright configuration/seeding + CI workflow copy steps to prevent tours from breaking existing e2e actionability.
File summaries
| File | Description |
|---|---|
| eform-client/src/app/plugins/modules/time-planning-pn/time-planning-pn.module.ts | Wires CDK overlay + declares help components in the plugin module. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-visibility.service.ts | Adds central “help visible” gate driven by ngrx admin selector. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-visibility.service.spec.ts | Unit tests for help visibility gating behavior. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-tour.service.ts | Adds tour sequencing + seen-state persistence. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-tour.service.spec.ts | Unit tests for tour sequencing, anchor skipping, and persistence. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-search.service.ts | Adds help search with diacritic folding + locale/English fallback ranking. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-search.service.spec.ts | Unit tests for search ranking, folding, and fallback behavior. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-panel.service.ts | Adds service for global panel open/target/surface state. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-panel.service.spec.ts | Unit tests for help panel service state updates. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-content.service.ts | Adds registry-backed content resolver with per-entry fallback. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/services/help-content.service.spec.ts | Unit tests for locale resolution + admin-only filtering. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/playwright-tour-seed.spec.ts | Guards Playwright storage seeding and config wiring for tours. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/planning-help.registry.ts | Defines the planning help registry entries (tasks/controls/tours). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/planning-help.registry.spec.ts | Registry integrity tests (ids, prose coverage, tour step rules, etc.). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/i18n/index.ts | Registers help locales + fallback mappings. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/i18n/enUS.ts | Adds English help prose + UI chrome strings. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/i18n/day-type-copy.spec.ts | Cross-locale tests for day-type copy correctness constraints. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/i18n/da.spec.ts | Danish help content tests (coverage, keywords, copy rules). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/help.model.ts | Adds the help domain model (ids, entry types, UI strings). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/help-wiring.spec.ts | Integration-style wiring tests (anchors, gating, overlay z-index, etc.). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/grid-help-anchors.ts | Adds DOM stamping for the grid sort header anchor. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-tour/help-tour.component.ts | Adds the guided-tour overlay component implementation. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-tour/help-tour.component.spec.ts | Unit tests for tour overlay rendering and behavior. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-tour/help-tour.component.scss | Styles for the guided-tour overlay card. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-tour/help-tour.component.html | Template for tour overlay rendering and actions. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-panel/help-panel.component.ts | Adds the searchable side-panel component with overlay-container parking. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-panel/help-panel.component.spec.ts | Unit tests for panel behavior, gating, deep-linking, and keyboard handling. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-panel/help-panel.component.scss | Styles for the help side panel + entry rendering. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-panel/help-panel.component.html | Template for panel browsing/search/results + related links. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-icon/help-icon.component.ts | Adds the ⓘ popover help icon component (overlay + close-on-scroll). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-icon/help-icon.component.spec.ts | Unit tests for popover behavior, events, and gating. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-icon/help-icon.component.scss | Styles for the help icon and popover, including name-column variant. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-icon/help-icon.component.html | Template for help icon trigger + connected overlay popover. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-hint/help-hint.component.ts | Adds inline help-hint component (info/warn tone). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-hint/help-hint.component.spec.ts | Unit tests for hint rendering, tone, and gating. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-hint/help-hint.component.scss | Styles for inline help hints (info/warn). |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-hint/help-hint.component.html | Template for inline help hint rendering. |
| eform-client/src/app/plugins/modules/time-planning-pn/help/components/help-chrome.base.ts | Adds shared base directives for help UI strings + gating + entry prose lookup. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-plannings-table/time-plannings-table.component.ts | Stamps grid header anchor and opens help panel from name-column icon. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-plannings-table/time-plannings-table.component.html | Adds name-column help icon, empty-grid hint, and many help anchors. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-plannings-container/time-plannings-container.component.ts | Wires panel/tours, adds ? button, and auto-starts page tour once. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-plannings-container/time-plannings-container.component.spec.ts | Adds tests for page tour offer rules and replay behavior. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-plannings-container/time-plannings-container.component.html | Adds help anchors, help ? button, mounts panel and page tour. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/workday-entity/workday-entity-dialog.component.ts | Wires dialog help icons/anchors and auto-starts dialog tour once. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/workday-entity/workday-entity-dialog.component.spec.ts | Adds tests ensuring dialog surface is passed and admin flag is forwarded. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/workday-entity/workday-entity-dialog.component.scss | Adds layout helper class for field+help icon pairing. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/workday-entity/workday-entity-dialog.component.html | Adds help icons/anchors, dialog hint(s), and mounts dialog tour. |
| eform-client/playwright/helpers/tour-seen.storage.json | Seeds tour “seen” state for Playwright runs. |
| eform-client/playwright.config.ts | Wires storage state seed to prevent auto-tours breaking e2e actions. |
| docs/superpowers/specs/2026-09-04-planning-help-system-design.md | Design document for the planning help system. |
| .gitignore | Ignores eform-client/node_modules. |
| .github/workflows/dotnet-core-pr.yml | Copies Playwright seed/config into frontend workspace for unit tests. |
| .github/workflows/dotnet-core-master.yml | Same as PR workflow: copies Playwright seed/config for unit tests. |
Review details
- Files reviewed: 53/55 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+52
to
+53
| import {OverlayModule} from '@angular/cdk/overlay'; | ||
| import {CdkScrollable} from '@angular/cdk/scrolling'; |
Comment on lines
+104
to
+108
| OverlayModule, | ||
| // The day-cell dialog declares its own overflow container inside | ||
| // mat-dialog-content; ScrollDispatcher only sees containers marked | ||
| // cdkScrollable, and the help popovers dismiss on scroll. | ||
| CdkScrollable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an in-page help system to
plugins/time-planning-pn/planning.The planning page is not admin-gated — any user holding
time_planning_plugin_accesscan edit almost everything on it, including the 2001-line day-cell editor — yet nothing on the page explained flex, pauses, netto hours, or what saving triggers. The 67 existingmatTooltips are icon labels, not explanations, and no help affordance of any kind existed in either repo.What this adds
One help registry of 50 entries — 12 how-to tasks and 38 control descriptions — in English and Danish. Four surfaces read from it, so support writes an answer once and it appears everywhere:
cdkConnectedOverlay, works inside the day-cellMatDialog.help-textpatternContent lives in
help/i18n/, so the 25 shared locale files are untouched. No new npm dependency. No backend change. No change toeform-angular-frontend.Why search needed a second entry kind
Nobody searches for a control named "vacation" — no such control exists. Control descriptions alone returned nothing for the most common question the page receives, so tasks carry ordered steps and per-locale keywords (ferie, sygdom, afspadsering, barsel appear in no English string).
The highest-value content is the day-flag rule, which the UI actively hides: the flags render as checkboxes but are mutually exclusive, and ticking one rewrites netto hours —
0for Day off and Vacation day off, the planned hours for every other type. The trap is naming, not adjacency: Time off keeps the hours, and in Danish Ferie fridag does too, despite being named a fridag.Rolled out to admins first
The whole system —
?button, panel, both tours, every ⓘ and hint — is currently visible only to admins. A staged rollout, not a change of audience: the content is written for the team lead and the copy rules still bind, because customers see it when the gate lifts.The gate sits at three choke points rather than ~20 template call sites, so no usage site can leak:
HelpVisibilityService(live subscription, nevertake(1)) read byHelpEntryChromeBase.prose,HelpTourService.start()andHelpPanelComponent. A tour refused by the gate is deliberately not marked seen, so nobody burns their one automatic offer while the gate is up.Testing
184 help tests across 14 suites. Note
angular-unit-testiscontinue-on-error: truein both workflows, so these are non-gating — their value is what they catch for a reader.Five vacuous assertions were found and rewritten during review, so new tests here are mutation-checked: each was verified to go red against a deliberate break of the thing it names.
playwright/helpers/tour-seen.storage.jsonseedstp.planning.tour.v1viause.storageState. Without it both tours auto-open a 320px overlay in every browser context, over the grid and over the shift-1 fields that 26 e2e specs click. Both workflows now copy that seed into the unit-test job so its guard has files to read.Design docs
docs/superpowers/specs/2026-09-04-planning-help-system-design.mdand the implementation plan beside it.Not verified in a browser
Overlay-above-
MatDialogstacking, and Escape-with-the-panel-open leaving the dialog's unsaved edits intact. Both are covered by tests and were traced through the CDK sources, but neither has been confirmed by a human in a running app.🤖 Generated with Claude Code