feat(workspace): expandable navigation rail#1070
Conversation
Greptile SummaryIntroduces an expandable/collapsible navigation rail to the workspace shell. The brand logo is converted from a navigation anchor to a toggle button, with
Confidence Score: 4/5Safe to merge; the toggle and compact-guard logic is well-tested and the state propagation between parent and child is correct for all current call sites. The core expand/collapse flow is sound — MediaQueryList cleanup is properly tied to DestroyRef, ARIA attributes are correctly wired, and 102 tests pass. The three comments are all non-blocking: a documented style-guide deviation for host bindings, a duplicated 640px breakpoint across SCSS and TS, and a future-proofing note about the model() compact guard being bypassable from the parent. None affect current behavior. The rail component TypeScript file is the most nuanced: it owns the MediaQueryList listener, the model() two-way binding, and the host class binding that deviates from the project style guide. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BrandButton as Brand Button (toggle)
participant Rail as WorkspaceShellRailComponent
participant Shell as WorkspaceShellComponent
participant MediaQuery as MediaQueryList (640px)
User->>BrandButton: click
BrandButton->>Rail: toggleExpanded()
alt "isCompact() == true"
Rail-->>BrandButton: (no-op, button disabled)
else "isCompact() == false"
Rail->>Rail: expanded.update(!expanded)
Rail-->>Shell: "(expandedChange) = true/false"
Shell->>Shell: railExpanded.set($event)
Shell->>Rail: "[expanded]="railExpanded()""
Shell->>Shell: [class.rail-expanded] applied to grid
end
MediaQuery->>Rail: "onMediaChange({ matches: true })"
Rail->>Rail: isCompact.set(true)
Rail->>Rail: expanded.set(false)
Rail-->>Shell: "(expandedChange) = false"
Shell->>Shell: railExpanded.set(false)
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts:21-25
The `CLAUDE.md` style guide requires `@HostBinding()` decorators for host class bindings. The `host` metadata property is used here instead. While `host` metadata with signal expressions is increasingly idiomatic for Angular signals, the project's documented standard is `@HostBinding()`.
```suggestion
@Component({
selector: 'app-workspace-shell-rail',
```
### Issue 2 of 3
libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts:40-45
**Compact breakpoint duplicated across SCSS and TypeScript**
The `640px` threshold is defined separately in the TypeScript `matchMedia('(max-width: 640px)')` call and in both SCSS files (`workspace-shell-rail.component.scss` and `workspace-shell-rail-links.component.scss`) via `@media (max-width: 640px)`. If the breakpoint ever needs to change, all three sites must be updated in sync. Consider extracting this to a shared SCSS variable or a typed constant imported by the TS side.
### Issue 3 of 3
libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts:44-82
**Compact-guard only protects `toggleExpanded()`, not the model input**
The `isCompact()` check in `toggleExpanded()` prevents button clicks from expanding the rail on narrow viewports. However, `expanded` is a `model()`, meaning the parent can write `true` into it directly via the `[expanded]` binding — bypassing the guard entirely. Currently the parent's `railExpanded` signal only ever becomes `true` through `expandedChange`, so no real bug exists today. But if a future caller sets `railExpanded` to `true` independently (e.g., from a keyboard shortcut or deep-link restoration), the rail would render as expanded on mobile with no way to collapse it. A defensive `effect()` that resets `expanded` to `false` whenever both `isCompact()` and `expanded()` are `true` would make the invariant robust.
Reviews (1): Last reviewed commit: "fix(i18n): clarify Arabic navigation lab..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @Component({ | ||
| selector: 'app-workspace-shell-rail', | ||
| host: { | ||
| '[class.rail-expanded]': 'expanded()', | ||
| }, |
There was a problem hiding this comment.
CLAUDE.md style guide requires @HostBinding() decorators for host class bindings. The host metadata property is used here instead. While host metadata with signal expressions is increasingly idiomatic for Angular signals, the project's documented standard is @HostBinding().
| @Component({ | |
| selector: 'app-workspace-shell-rail', | |
| host: { | |
| '[class.rail-expanded]': 'expanded()', | |
| }, | |
| @Component({ | |
| selector: 'app-workspace-shell-rail', |
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts
Line: 21-25
Comment:
The `CLAUDE.md` style guide requires `@HostBinding()` decorators for host class bindings. The `host` metadata property is used here instead. While `host` metadata with signal expressions is increasingly idiomatic for Angular signals, the project's documented standard is `@HostBinding()`.
```suggestion
@Component({
selector: 'app-workspace-shell-rail',
```
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/iptvnator/github/4gray/iptvnator/-/custom-context?memory=d7399d0f-9d03-4561-9aed-4525c6d78ad1))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private readonly compactMediaQuery = | ||
| this.document.defaultView?.matchMedia?.('(max-width: 640px)') ?? null; | ||
|
|
||
| readonly isMacOS = input(false); | ||
| readonly brandLink = input('/workspace/dashboard'); | ||
| readonly brandTooltipKey = input('WORKSPACE.SHELL.RAIL_DASHBOARD'); | ||
| readonly brandAriaLabelKey = input('WORKSPACE.SHELL.OPEN_DASHBOARD'); | ||
| readonly expanded = model(false); | ||
| readonly isCompact = signal(this.compactMediaQuery?.matches ?? false); |
There was a problem hiding this comment.
The 640px threshold is defined separately in the TypeScript matchMedia('(max-width: 640px)') call and in both SCSS files (workspace-shell-rail.component.scss and workspace-shell-rail-links.component.scss) via @media (max-width: 640px). If the breakpoint ever needs to change, all three sites must be updated in sync. Consider extracting this to a shared SCSS variable or a typed constant imported by the TS side.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts
Line: 40-45
Comment:
**Compact breakpoint duplicated across SCSS and TypeScript**
The `640px` threshold is defined separately in the TypeScript `matchMedia('(max-width: 640px)')` call and in both SCSS files (`workspace-shell-rail.component.scss` and `workspace-shell-rail-links.component.scss`) via `@media (max-width: 640px)`. If the breakpoint ever needs to change, all three sites must be updated in sync. Consider extracting this to a shared SCSS variable or a typed constant imported by the TS side.
How can I resolve this? If you propose a fix, please make it concise.| @@ -39,4 +51,33 @@ export class WorkspaceShellRailComponent { | |||
| >(null); | |||
| readonly railProviderClass = input('rail-context-region'); | |||
| readonly isSettingsRoute = input(false); | |||
|
|
|||
| constructor() { | |||
| const mediaQuery = this.compactMediaQuery; | |||
| if (!mediaQuery) { | |||
| return; | |||
| } | |||
|
|
|||
| const updateCompactState = (matches: boolean): void => { | |||
| this.isCompact.set(matches); | |||
| if (matches) { | |||
| this.expanded.set(false); | |||
| } | |||
| }; | |||
| const onMediaChange = (event: MediaQueryListEvent): void => | |||
| updateCompactState(event.matches); | |||
|
|
|||
| mediaQuery.addEventListener?.('change', onMediaChange); | |||
| this.destroyRef.onDestroy(() => | |||
| mediaQuery.removeEventListener?.('change', onMediaChange) | |||
| ); | |||
| } | |||
|
|
|||
| toggleExpanded(): void { | |||
| if (this.isCompact()) { | |||
| return; | |||
| } | |||
|
|
|||
| this.expanded.update((expanded) => !expanded); | |||
| } | |||
There was a problem hiding this comment.
toggleExpanded(), not the model input
The isCompact() check in toggleExpanded() prevents button clicks from expanding the rail on narrow viewports. However, expanded is a model(), meaning the parent can write true into it directly via the [expanded] binding — bypassing the guard entirely. Currently the parent's railExpanded signal only ever becomes true through expandedChange, so no real bug exists today. But if a future caller sets railExpanded to true independently (e.g., from a keyboard shortcut or deep-link restoration), the rail would render as expanded on mobile with no way to collapse it. A defensive effect() that resets expanded to false whenever both isCompact() and expanded() are true would make the invariant robust.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/workspace/shell/feature/src/lib/workspace-shell/components/workspace-shell-rail/workspace-shell-rail.component.ts
Line: 44-82
Comment:
**Compact-guard only protects `toggleExpanded()`, not the model input**
The `isCompact()` check in `toggleExpanded()` prevents button clicks from expanding the rail on narrow viewports. However, `expanded` is a `model()`, meaning the parent can write `true` into it directly via the `[expanded]` binding — bypassing the guard entirely. Currently the parent's `railExpanded` signal only ever becomes `true` through `expandedChange`, so no real bug exists today. But if a future caller sets `railExpanded` to `true` independently (e.g., from a keyboard shortcut or deep-link restoration), the rail would render as expanded on mobile with no way to collapse it. A defensive `effect()` that resets `expanded` to `false` whenever both `isCompact()` and `expanded()` are `true` would make the invariant robust.
How can I resolve this? If you propose a fix, please make it concise.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1070 +/- ##
===========================================
- Coverage 71.05% 54.65% -16.41%
===========================================
Files 40 538 +498
Lines 691 29918 +29227
Branches 87 6409 +6322
===========================================
+ Hits 491 16352 +15861
- Misses 176 11090 +10914
- Partials 24 2476 +2452
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The settings.e2e "starts on sources when dashboard is disabled" test and the saveSettings fixture switched a.brand -> button.brand / "Dashboard"|"Sources" nav-rail links. That selector update belongs to the navigation-rail PR (4gray#1070), not this xtream-only branch — which renders upstream's a.brand and so must keep upstream's e2e selectors. Reverted both e2e files to upstream/master.
The rail replaces the brand anchor (a.brand) with button.brand and exposes "Dashboard"/"Sources" as nav-rail links. Update the "starts on sources when dashboard is disabled" test and the saveSettings fixture to match the new rail markup (getByRole link + is-active / button.brand visible). These e2e selector updates pair with this PR's rail change; without them the rail breaks the upstream a.brand-based selectors.
|
Closing — consolidating this split back into #1056 at the author's request. Superseded by the combined PR. |
2 participants
Summary
Adds an expandable navigation rail to the workspace shell, split out of #1056 (workspace topic only).
workspace-shell-rail,workspace-shell-rail-links): a compact icon rail that expands to show labelled navigation, with workspace + primary/secondary context links.railExpandedsignal on the shell drives a.rail-expandedclass and the rail's[expanded]/(expandedChange)two-way binding.workspace-shell-route-state.service) and facade wiring for the rail's selected-section and provider styling.ar) navigation strings.docs/architecture/workspace-shell.md.Tests
workspace-shell-rail,workspace-shell-rail-links, facade, andworkspace-shell.componentspecs cover the compact↔expanded toggle and rail composition. Verified locally on top ofupstream/master: affectedbuild+lintclean;workspace-shell-featuretests 102/102 green.Context
Part of splitting the oversized #1056 into reviewable, topic-scoped PRs. Companion PRs: #1069 casting controls and xtream IPC hardening.