feat(shortcuts): global New create shortcut (LFXV2-2721)#1121
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a permission-scoped Create quick-link to the lens rail, a grouped artifact menu, a project-selection dialog with navigation setup, curated selector behavior, impersonation-aware positioning, and Playwright coverage for the complete flow. ChangesCreate quick-link flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LensRail as Lens rail
participant PermissionService as Permission service
participant CreateDialog as Create dialog
participant ProjectSelector as Project selector
participant Router
User->>LensRail: Click Create
LensRail->>PermissionService: Read creatable artifact types
LensRail->>CreateDialog: Open selected artifact
CreateDialog->>ProjectSelector: Render eligible projects
User->>ProjectSelector: Select project
ProjectSelector->>CreateDialog: Return selected item
CreateDialog->>Router: Navigate to artifact create route
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a permission-gated desktop rail shortcut for creating meetings, groups, and mailing lists within a selected project or foundation.
Changes:
- Adds shared artifact configuration and project-selection types.
- Adds writer/lens eligibility, popover, dialog, and context-aware navigation.
- Adds Playwright smoke coverage for the shortcut flow.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/interfaces/index.ts |
Exports quick-link interfaces. |
packages/shared/src/interfaces/create-artifact.interface.ts |
Defines artifact and target-project types. |
packages/shared/src/constants/index.ts |
Exports artifact configuration. |
packages/shared/src/constants/create-artifact.constants.ts |
Configures supported artifact types and routes. |
apps/lfx-one/src/app/shared/services/create-permission.service.ts |
Resolves writer- and lens-eligible projects. |
apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.ts |
Controls the menu and creation dialog. |
apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.scss |
Positions and styles the popover. |
apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.html |
Adds the desktop rail button and menu. |
apps/lfx-one/src/app/shared/components/create-artifact-dialog/create-artifact-dialog.component.ts |
Aligns context and navigates to creation flows. |
apps/lfx-one/src/app/shared/components/create-artifact-dialog/create-artifact-dialog.component.html |
Renders project selection and actions. |
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.html |
Accounts for the impersonation banner offset. |
apps/lfx-one/e2e/create-quick-link.spec.ts |
Adds shortcut smoke tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function skipWhenNoCreatePermission(page: Page): Promise<void> { | ||
| const trigger = page.getByTestId('create-rail-button'); | ||
| const visible = await trigger.isVisible().catch(() => false); | ||
| if (!visible) { | ||
| test.skip(true, 'Test user holds `writer` on no project — button hidden by design.'); |
There was a problem hiding this comment.
Acknowledged — because the suite skips when the test account holds no writer grant, a render/permission/eligibility regression could pass silently. Making the positive path non-skippable needs a page.route stub of /api/projects (a deliberate shift from this suite's real-API, name-agnostic design) plus a separate fail-closed case. Tracking as a follow-up rather than expanding this PR — leaving this thread open.
…LFXV2-2721) Adds a "New" (+) button to the left nav rail that opens a popover to create a Meeting, Group, or Mailing List. A dialog picks the target Foundation/Project, then routes into the existing create flows with that context pre-selected. Eligibility is the intersection of the project writer grant, the same signal writerGuard enforces, and a lens the user's persona holds. GET /api/projects batch access-checks every visible project and returns writer per project. activeContext is lens-gated, so a project outside the user's lenses could not be resolved by the create page; writer and lens are independent grants, so both must hold. Everything resolves empty while loading and on error, keeping the button hidden until eligibility is proven. Applied on top of the attached patch: - Gate on the writer grant rather than a persona proxy. The patch inferred eligibility, and isFoundation, from which persona bucket a project arrived in, which mislabelled every ED-bucket project as a foundation and left parent_uid and logoUrl unset. isFoundation now comes from computeIsFoundation over the project's own attributes. - Align the lens before seeding context, and abort if setLens refuses, so the create page cannot resolve a different project than the one picked. - Pass syncUrl false, so seeding no longer rewrites the history entry of the page the dialog was opened from. - Navigate with ?project= so writerGuard is authoritative on the selection. - Load the project list in afterNextRender. The request is already issued during SSR by profile-affiliations, so the transfer cache would resolve it synchronously at construction and render the button on the client's first pass while the server rendered it absent. - Offset the popover under the impersonation banner, which shifts the rail down. The bottom-anchored apps and user menus are immune to this; a top-anchored one is not. - Report the dialog's dead ends with a toast instead of an inert CTA. - Target data-test for the project select in e2e; lfx-select does not emit data-testid, so the assertion could never run. - Use fa-light for the rail icon to match the other rail affordances. Co-authored-by: Nuno Eufrasio <neufrasio@contractor.linuxfoundation.org> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Adds Newsletter as a fourth type in the rail Create quick-link. Newsletter creation is writer-gated by newsletterAccessGuard (ED or project.writer === true), the same signal CreatePermissionService already intersects on, so the entry never dead-ends an offered project. Uses the newsletters feature's own fa-paper-plane icon and the existing /newsletters/create route. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Expands the rail Create quick-link from four types to six, in a deliberate sequence: Meeting, Newsletter, Vote, Survey, Group, Mailing List. The order is a semantic grouping, not frequency — Engage (Meeting, Newsletter), Decide (Vote, Survey), Organize (Group, Mailing List). A thin separator is drawn wherever the group changes, driven by a new group field on CREATABLE_ARTIFACTS so sequence and grouping share one source of truth. No group labels yet. Vote and Survey create routes carry the same authGuard + writerGuard (writeFeature: votes/surveys) as the existing four, and CreatePermissionService already intersects on project.writer, so neither entry over-promises a route the persona cannot use. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Full-branch sweep flagged (general + conventions reviewers) that creatableTypes is all-or-nothing, so the per-type filter it feeds in lens-switcher never removes an entry — presenting the illusion of per-type control. Add a doc comment warning maintainers that a new CREATABLE_ARTIFACTS entry inherits visibility for free and does not get independent gating, and note the filter is forward-looking scaffolding for a future GET /api/projects/creatable endpoint. No behavior change. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…oped list Replaces the plain PrimeNG select in the create-artifact dialog with the existing lfx-project-selector, so the dialog presents the familiar sidebar pattern: search, All/Foundations/Projects tabs, logos, and role badges. The selector defaults to NavigationService, which is view-scoped (every project the user can see, gated only by the bearer token) — adopting it as-is would have listed the global catalog. To keep the create list scoped to projects the user holds writer on, extend lfx-project-selector with an optional curated `items` input: when provided it renders that list with local search + tab filtering and no pagination; when absent (every sidebar caller) nav-backed behavior is unchanged. The dialog feeds it the writer-scoped creatableProjects mapped to LensItem, so scope is unchanged from before — only the UI changed. Also aligns the dialog label color to the gray brand scale (slate is not an lfx scale) and updates the e2e to drive the selector: asserts search + tabs render and that sentinel non-member catalog names are absent. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…ames
Post-commit trio flagged two e2e issues (production code was clean):
- General reviewer: getByTestId('project-selector') was unscoped, but that testid
is emitted by the lfx-project-selector template itself, so the sidebar's instance
(present in project/foundation lens — the writer-scoped test user's normal state)
also matches it, tripping Playwright strict mode. Scope the trigger to the dialog.
- Conventions reviewer: S4 hardcoded catalog names ('AAA'/'QHTTPX') to assert a
negative, which is vacuous and violates this suite's real-API, name-agnostic
convention. Replace with a contract assertion (search + tabs + a selectable
lens-item render); writer-scoping is guaranteed by the dialog feeding the curated
writer-scoped list, verified in the production-code review.
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Post-commit trio (all three) flagged the S4 coverage-map line still claiming it lists 'only writer-scoped projects, not the global catalog' — the exact catalog- exclusion assertion the prior commit removed. Update the docstring to describe what S4 now verifies (search + tabs + a selectable list); writer-scoping is guaranteed by the dialog feeding the curated creatableProjects, verified in production-code review, not asserted in-suite. Comment-only. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Full-branch sweep findings: - Conventions reviewer: selectedContext (WritableSignal) was declared after the derived projectOptions signal; component-organization.md wants writables before derived. Move selectedContext above the derived signals. No behavior change. - General reviewer: the Create button gates purely on the FGA writer relation with no ED fast-path, so an ED with no per-project writer grant sees no shortcut. Per product decision (LFXV2-2721) this stays writer-scoped; document the ED persona-differential explicitly in the service docstring as an accepted trade-off (EDs create via the in-page flows, which carry their own ED fast-path). Doc-only. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
When the user holds writer on exactly one project/foundation, the create dialog pre-selects it on open so Continue is enabled immediately and the picker shows the locked-in choice — no redundant open-and-pick for a list of one. Extracts the selection logic into applySelection(), called from the constructor when creatableProjects has a single entry and from onItemSelected otherwise. If the list later widens (persona data resolving), the user can still change the pick. Adds e2e S5, which adapts to the real-API test account: asserts Continue is enabled on open when exactly one project is eligible, and gated until a pick when several are. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…elect General reviewer flagged that S3 asserted Continue disabled immediately on dialog open, but the auto-select commit enables Continue on open for a single-eligible- project account — so S3 would fail where S5 passes on that account shape. Remove the on-open assertion (S5 now owns the enabled/disabled-on-open distinction); S3 keeps its real job: picking a project via the selector enables Continue. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
General reviewer (full-branch sweep) flagged the ProjectContext object being rebuilt field-by-field in both onContinue and applySelection. Extract a single toContext(project) helper so the field list lives in one place and can't drift if ProjectContext gains a field. No behavior change. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
2430104 to
18f0aeb
Compare
manishdixitlfx
changed the title
| await page | ||
| .getByTestId('create-rail-button') | ||
| .waitFor({ state: 'visible', timeout: RAIL_TIMEOUT }) | ||
| .catch(() => undefined); | ||
| await skipWhenNoCreatePermission(page); |
There was a problem hiding this comment.
Acknowledged — because the suite skips when the test account holds no writer grant, a render/permission/eligibility regression could pass silently. Making the positive path non-skippable needs a page.route stub of /api/projects (a deliberate shift from this suite's real-API, name-agnostic design) plus a separate fail-closed case. Tracking as a follow-up rather than expanding this PR — leaving this thread open.
PR SummaryMedium Risk Overview Flow: rail popover lists six creatable types from shared Supporting changes: Note: Shortcut is UX-only—route Reviewed by Cursor Bugbot for commit 8853ae6. Bugbot is set up for automated code reviews on this repo. Configure here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/e2e/create-quick-link.spec.ts`:
- Around line 55-61: Replace skipWhenNoCreatePermission’s UI-based entitlement
check with writer eligibility derived independently from the projects response
or a deterministic fixture, skipping only when that source confirms no eligible
projects. In apps/lfx-one/e2e/create-quick-link.spec.ts lines 98-114, derive
expected artifact types from the eligible lenses instead of assuming six; in
lines 138-158, compare rendered projects against the independently derived
writer-eligible project set. Update all three sites while preserving valid
partial-eligibility assertions and preventing writer-scope leaks.
In
`@apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.scss`:
- Line 80: Add the same targeted Stylelint suppression for
selector-pseudo-element-no-unknown to both ::ng-deep rules in the lens-switcher
component stylesheet, including the .lfx-create-popover.p-popover and the other
Angular deep selector, without changing their styling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83580761-c1ba-4d55-9756-e604db9e8eb1
📒 Files selected for processing (13)
apps/lfx-one/e2e/create-quick-link.spec.tsapps/lfx-one/src/app/layouts/main-layout/main-layout.component.htmlapps/lfx-one/src/app/shared/components/create-artifact-dialog/create-artifact-dialog.component.htmlapps/lfx-one/src/app/shared/components/create-artifact-dialog/create-artifact-dialog.component.tsapps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.htmlapps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.scssapps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.tsapps/lfx-one/src/app/shared/components/project-selector/project-selector.component.tsapps/lfx-one/src/app/shared/services/create-permission.service.tspackages/shared/src/constants/create-artifact.constants.tspackages/shared/src/constants/index.tspackages/shared/src/interfaces/create-artifact.interface.tspackages/shared/src/interfaces/index.ts
| // Skip when the test user has no create permission — the button is intentionally absent. | ||
| async function skipWhenNoCreatePermission(page: Page): Promise<void> { | ||
| const trigger = page.getByTestId('create-rail-button'); | ||
| const visible = await trigger.isVisible().catch(() => false); | ||
| if (!visible) { | ||
| test.skip(true, 'Test user holds `writer` on no project — button hidden by design.'); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Derive expected permissions independently from the UI under test.
The suite currently has no entitlement oracle, allowing regressions to be skipped, valid partial eligibility to fail, and writer-scope leaks to pass.
apps/lfx-one/e2e/create-quick-link.spec.ts#L55-L61: determine whether the user has writer eligibility from the projects response or a deterministic fixture, not button visibility.apps/lfx-one/e2e/create-quick-link.spec.ts#L98-L114: derive the expected artifact types from eligible lenses rather than always expecting six.apps/lfx-one/e2e/create-quick-link.spec.ts#L138-L158: compare rendered projects with the independently derived writer-eligible project set.
📍 Affects 1 file
apps/lfx-one/e2e/create-quick-link.spec.ts#L55-L61(this comment)apps/lfx-one/e2e/create-quick-link.spec.ts#L98-L114apps/lfx-one/e2e/create-quick-link.spec.ts#L138-L158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/lfx-one/e2e/create-quick-link.spec.ts` around lines 55 - 61, Replace
skipWhenNoCreatePermission’s UI-based entitlement check with writer eligibility
derived independently from the projects response or a deterministic fixture,
skipping only when that source confirms no eligible projects. In
apps/lfx-one/e2e/create-quick-link.spec.ts lines 98-114, derive expected
artifact types from the eligible lenses instead of assuming six; in lines
138-158, compare rendered projects against the independently derived
writer-eligible project set. Update all three sites while preserving valid
partial-eligibility assertions and preventing writer-scope leaks.
There was a problem hiding this comment.
Acknowledged — an independent entitlement oracle (stub /api/projects, derive expected types from eligible lenses, compare rendered vs writer-eligible set) is a real improvement, but a heavier rework that flips this suite from real-API to stubbed. Tracking as a follow-up — leaving open.
There was a problem hiding this comment.
@manishdixitlfx Understood—keeping this review open. Would you like me to create and assign a GitHub follow-up issue for the independent entitlement-oracle coverage?
audigregorie
left a comment
audigregorie
left a comment
There was a problem hiding this comment.
Code Review Summary
A well-structured feature: fail-closed writer-gating, a clean items input extension to project-selector, and an honest, thorough trade-off write-up. The main concerns are (1) the writer list is fetched exactly once with no retry/refresh, (2) global lens/context state is mutated before the route guard authorizes, and (3) the popover position is hard-coded layout math. None block merge, but #1 and #2 are worth addressing or documenting as trade-offs.
Major — outside the diff
- Full-catalog scan on every app load —
getProjects()pulls every visible project and batch access-checks it; the rail icon lags until that resolves on a cold load (no transfer cache). Acknowledged as a follow-up (GET /api/projects/creatable). computeIsFoundationvs sidebarisFoundation— the dialog derivesisFoundationclient-side viacomputeIsFoundation(project)(funding/model heuristics), while the sidebar's lens-items endpoint carries its ownisFoundation. If the two ever disagree for the same project, the dialog will dispatch a project throughsetFoundation()+setLens('foundation')and route it to the foundation-lens create mount. Low probability, worth a mental check.getProjects()cache vs impersonation —ProjectService.projectsCacheis keyed by params and lives on the root service; if it isn't cleared on impersonation start/stop, the create button (and the writer list) reflect the pre-impersonation user for the whole impersonation session. Worth verifying the impersonation flow invalidates this cache.
What's done well
- Fail-closed visibility gating is the right default, and the writer-intersection-with-lens reasoning is documented honestly, including the ED trade-off and the
meeting_coordinatorunder-show. - Reusing
project-selectorvia a curateditemsinput (with nav-backed behavior unchanged whennull) is a clean extension that avoids forking the selector UI. - E2E suite is name-agnostic, skips cleanly when the test user has no writer grant, and the coverage-map docstring maps assertions to S1-S5.
| // request during SSR, so the transfer cache would resolve it synchronously here and render | ||
| // the button on the client's first pass while the server rendered it absent — a hydration | ||
| // mismatch. `afterNextRender` runs browser-only, once the first render is committed. | ||
| afterNextRender(() => { |
There was a problem hiding this comment.
Major — writer list fetched exactly once; no retry and no refresh
afterNextRender fires a single time and the subscribe sets writerProjects once. The class doc claims the button is hidden "until eligibility is proven", but a transient getProjects() failure (the doc itself notes the error path relies on the BFF emitting []) leaves writerProjects = [] for the entire session — indistinguishable from "no permission". The same staleness applies to mid-session grant changes and impersonation: ProjectService.getProjects() is shareReplay(1)-cached per param key, and nothing here re-fetches when the user's grants actually change. The follow-up GET /api/projects/creatable note acknowledges latency, but the once-only subscription is a correctness gap, not just perf.
Consider re-subscribing on a persona/impersonation change signal, or at minimum retrying once on an empty result that may be transient.
There was a problem hiding this comment.
Agreed this is a correctness gap, not just perf — a transient getProjects() failure hides the button for the whole session, and grant/impersonation changes don't refresh (afterNextRender fires once; getProjects() is shareReplay-cached). It's the same surface as the deferred optimistic/refresh work and the GET /api/projects/creatable follow-up; grouping them in that PR rather than expanding this one. Leaving open.
| // persona doesn't hold — `writer` and lens roles are independent grants — and there is no way | ||
| // to align the context in that case, so bail before mutating anything rather than create | ||
| // against a stale project (writerGuard's ED fast-path would not catch it). | ||
| if (!this.lensService.setLens(project.isFoundation ? 'foundation' : 'project')) { |
There was a problem hiding this comment.
Major — global lens + context mutated before writerGuard authorizes
onContinue calls setLens(...) then setProject/setFoundation(...) (both persist cookies / mutate selectedLens + navLensSelection) before router.navigate triggers the destination's writerGuard. The inline comment says "bail before mutating anything", but that only covers the setLens-returns-false branch — a downstream guard denial (or a cancelled/replaced navigation) leaves the user's prior lens and project context already overwritten. Today the denial path is nearly unreachable because writerGuard's getProject(slug) is also shareReplay-cached off the same writer flag the dialog used, but the mutation order is still fragile.
Prefer seeding the slug via the ?project= query param and letting projectQueryParamGuard + the create page do the lens/context alignment after the guard resolves, or roll these mutations into a router.navigate resolved-handler.
There was a problem hiding this comment.
Good point on the mutation order. We already navigate with ?project=<slug> (so writerGuard is authoritative on the target), but setLens/setProject do persist before the guard resolves — a denial would leave stale lens/context. Reworking to seed via the query param + a resolved-handler is a fair follow-up; today the denial path is near-unreachable (same shared writer flag), so I'd rather not expand this PR's surface. Leaving open for the follow-up.
| top: 58px !important; | ||
|
|
||
| // stylelint-disable-next-line declaration-no-important | ||
| bottom: auto !important; | ||
|
|
||
| // stylelint-disable-next-line declaration-no-important | ||
| margin-top: 0 !important; | ||
| } | ||
|
|
||
| // Applied when the host layout declares it shifts the rail down to clear a banner — `main-layout` | ||
| // under impersonation (`top-12`, 48px), taking the Create button with it. Driven by the | ||
| // `bannerOffset` input rather than the impersonation flag, because the shift is layout-local: the | ||
| // docs shell pins the rail at `top-0` and renders no banner even while impersonating. 58 + 48 = 106px. | ||
| // Both classes are stacked deliberately: matching only `.lfx-create-popover-banner-offset` ties | ||
| // the base rule on specificity, so this would win on source order alone and silently revert to | ||
| // 58px if either block moved. | ||
| ::ng-deep .lfx-create-popover.lfx-create-popover-banner-offset.p-popover { | ||
| // stylelint-disable-next-line declaration-no-important | ||
| top: 106px !important; |
There was a problem hiding this comment.
Major — popover position is hard-coded layout math
top: 58px (and 106px for the banner variant) are derived by hand from rail padding + logo height + margins. Any change to the logo size, rail padding, or a new banner height silently misaligns the popover from its trigger, and bannerOffset only covers the impersonation case — other rail shifts aren't covered. The bottom-anchored apps/user menus get away with this because they pin to the bottom; a top-anchored panel is sensitive to everything above it.
Consider measuring the trigger's getBoundingClientRect().top on open — project-selector already does this in alignPanelTop() — instead of hard-coding the offset.
There was a problem hiding this comment.
Agreed the hardcoded top: 58px/106px is fragile. Measuring getBoundingClientRect().top like project-selector.alignPanelTop() is the right fix; grouping it with the popover-positioning follow-up rather than this PR. Leaving open.
Address review comments from @audigregorie, copilot[bot], coderabbitai[bot]: - create-artifact dialog: name the role=dialog via ariaLabelledBy -> the body's <h2 id=create-artifact-heading> (showHeader:false emits no title) (per copilot[bot]) - lens-switcher.scss: suppress stylelint selector-pseudo-element-no-unknown on both ::ng-deep rules so CI stylelint passes (per coderabbitai[bot]) - lens-switcher.html: aria-label 'New' -> 'Create new artifact' (per @audigregorie) - project-selector items input: document the curated-mode divergence — role badges and All-tab nesting key off persona-detection, so writer-only items render flat/unbadged (per @audigregorie) - e2e S6: assert Continue routes to the lens-prefixed create URL carrying ?project=<slug> (per copilot[bot]) - lens-switcher.html: document the shortcut is desktop-rail only by design; mobile deferred (per copilot[bot]) Deferred to follow-up (responded on-thread, threads left open): entitlement-oracle e2e stub, once-only writer fetch retry/refresh, lens/context mutation-before-guard, and dynamic popover positioning. Resolves 10 of 16 review threads. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Review Feedback AddressedCommit: Changes Made
No Change Needed
Threads Resolved10 of 16 threads resolved this iteration. Still Open (deferred to a follow-up PR)
These four are grouped into the follow-up PR (optimistic-load / latency / positioning) rather than expanding this one's surface. |
| public readonly creatableProjects: Signal<CreatableProject[]> = computed(() => { | ||
| const lenses = this.lensService.availableLenses(); | ||
| return this.writerProjects().filter((project) => lenses.some((lens) => lens.id === (project.isFoundation ? 'foundation' : 'project'))); |
There was a problem hiding this comment.
Acknowledged — before persona resolves, availableLenses() uses the default persona, so there's a brief window where a writer project could render under the wrong lens. This is the same persona-readiness/loading surface as the deferred optimistic-load + refresh work; gating on personaLoaded() also trades against the icon-load latency we're separately addressing. Grouping it into that follow-up rather than expanding this PR. Leaving open.
| async function skipWhenNoCreatePermission(page: Page): Promise<void> { | ||
| const trigger = page.getByTestId('create-rail-button'); | ||
| const visible = await trigger.isVisible().catch(() => false); | ||
| if (!visible) { | ||
| test.skip(true, 'Test user holds `writer` on no project — button hidden by design.'); | ||
| } |
There was a problem hiding this comment.
Same as the entitlement-oracle thread above — making the positive path non-skippable needs a page.route stub of /api/projects (a deliberate shift from this suite's real-API, name-agnostic design) plus a fail-closed case. Tracked as a follow-up. Leaving open.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // The slug keeps writerGuard authoritative on the chosen project and lets | ||
| // projectQueryParamGuard re-seed real project data on the lens-prefixed mounts. | ||
| this.router.navigate([this.artifact.createRoute], { queryParams: { project: project.slug } }); | ||
| this.dialogRef.close(true); |
There was a problem hiding this comment.
Context set before guards
Medium Severity
onContinue calls setLens, then setFoundation/setProject, then router.navigate. Route guards such as writerGuard run after those mutations. If navigation is denied, the user is redirected but lens and project context may already match the attempted create target instead of where they started.
Reviewed by Cursor Bugbot for commit ec90b29. Configure here.
There was a problem hiding this comment.
Acknowledged (same as the mutation-order thread): onContinue persists lens/context before writerGuard runs, so a denial could leave the create-target context. We navigate with ?project= (guard authoritative) and the denial path is near-unreachable today (shared writer flag), but reworking to seed via the query param + a resolved-handler is the right fix — tracked in the follow-up. Leaving open.
| takeUntilDestroyed(this.destroyRef) | ||
| ) | ||
| .subscribe((projects) => this.writerProjects.set(projects)); | ||
| }); |
There was a problem hiding this comment.
Create eligibility never refreshes
Medium Severity
The rail “Create” affordance loads writer projects once inside afterNextRender via a single getProjects() subscription. That observable is cached with shareReplay, so a failed or empty first emission keeps the button hidden for the whole SPA session, and later grant or impersonation changes are not picked up without a full reload.
Reviewed by Cursor Bugbot for commit ec90b29. Configure here.
There was a problem hiding this comment.
Acknowledged (same as the once-only-fetch thread): afterNextRender fires once and getProjects() is shareReplay-cached, so a transient failure hides the button for the session and grant/impersonation changes don't refresh. Grouped with the deferred optimistic-load/refresh + GET /api/projects/creatable work. Leaving open.
…lector Address PR #1121 review from copilot[bot]: initLensTypeLabel() checked only detectedProjects() then fell back to the active lens, so a curated (writer-scoped) entry not present in persona-detection could show the wrong kind — a foundation selected from a project page labelled 'Project' and vice versa. Resolve the selected uid from the curated items() (which carry isFoundation) before the active-lens fallback. Sidebar (non-curated) behavior unchanged. Resolves 1 review thread. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Review Feedback Addressed — iteration 2Commit: Changes Made
Threads Resolved1 of the 5 new threads this iteration. Still Open (deferred to the follow-up PR)The remaining threads restate four architectural items already tracked for the follow-up (optimistic-load / refresh / positioning / guard-ordering), now also raised by copilot[bot] and cursor[bot]:
These are grouped into the follow-up PR rather than expanding this one's surface; each has a rationale on-thread. |
| public readonly creatableProjects: Signal<CreatableProject[]> = computed(() => { | ||
| const lenses = this.lensService.availableLenses(); | ||
| return this.writerProjects().filter((project) => lenses.some((lens) => lens.id === (project.isFoundation ? 'foundation' : 'project'))); |
There was a problem hiding this comment.
Same persona-readiness point as the thread above — before persona resolves, availableLenses() uses the default (contributor) persona, so there's a brief window where a writer project could surface under the wrong lens (or an ED's foundation be filtered until persona loads). Gating on personaLoaded() also trades against the icon-load latency we're separately addressing, so it's grouped into the optimistic-load/refresh follow-up rather than this PR. Leaving open.
…ix assertion Address PR #1121 review from copilot[bot]: - project-selector: the panel forced sidebar-only 'position: fixed; left: 356px' at lg+, so in the create dialog the picker opened beside the sidebar instead of its trigger. Add a curated panel variant ('project-selector-panel-curated') with the cosmetics only (no fixed left/top) and skip alignPanelTop() in curated mode, so PrimeNG anchors the popover to its own trigger inside the dialog. Sidebar unchanged. - e2e S6: the URL regex also matched the flat /meetings/create, so a setLens/ lensRedirectGuard regression would still pass. Require the /(foundation|project)/ lens prefix explicitly. Resolves 2 review threads. Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Review Feedback Addressed — iteration 3Commit: Changes Made
Threads Resolved2 of the 3 new threads this iteration. Still Open (deferred to the follow-up PR)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/e2e/create-quick-link.spec.ts`:
- Around line 203-205: Replace the interpolated regex assertion in the
quick-link destination check with structural URL validation: parse page.url()
using URL, assert the pathname matches the required foundation/project
lens-prefixed meetings/create route, and compare searchParams.get('project')
directly with slug. Preserve the existing timeout and reject bare
/meetings/create paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83f2d536-21eb-43ff-9c4d-0455a917adc2
📒 Files selected for processing (4)
apps/lfx-one/e2e/create-quick-link.spec.tsapps/lfx-one/src/app/shared/components/project-selector/project-selector.component.htmlapps/lfx-one/src/app/shared/components/project-selector/project-selector.component.scssapps/lfx-one/src/app/shared/components/project-selector/project-selector.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/shared/components/project-selector/project-selector.component.ts
| type="button" | ||
| (click)="toggleCreateMenu($event)" | ||
| data-testid="create-rail-button" | ||
| aria-label="Create new artifact" |
| * Note: this suite stops at the dialog boundary. It does not assert the post-Continue | ||
| * create page — that path is enforced by each route's writerGuard. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 8853ae6. Configure here.
| const dialog = page.getByTestId('create-artifact-dialog'); | ||
| await dialog.getByTestId('project-selector').click(); | ||
| const panel = page.getByTestId('project-selector-panel'); | ||
| await expect(panel).toBeVisible({ timeout: 5_000 }); |
There was a problem hiding this comment.
E2E panel locator ambiguous
Medium Severity
The new create dialog reuses lfx-project-selector, so a second project-selector-panel node exists whenever the sidebar already shows the project picker. S3–S6 resolve the panel with a page-level getByTestId('project-selector-panel') while only scoping the trigger to the dialog. On /, the active lens often stays project or foundation from cookies, so Playwright can hit strict-mode violations or interact with the wrong panel.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 8853ae6. Configure here.
Deferred follow-ups tracked → LFXV2-2749Thanks all for the thorough review. The four architectural items intentionally deferred from this PR (to keep the initial feature scoped) are now tracked in LFXV2-2749:
All net-new findings from the review passes were addressed in-PR (a11y |
Summary
Adds a global "New" (+) shortcut to the left nav rail → popover of creatable types → a dialog to pick the target Foundation/Project → routes into the existing create flow with the context pre-selected.
writergrant (GET /api/projects→writer: true) and a lens the user's persona holds. Fail-closed — the button stays hidden until eligibility is proven, and resolves to empty on load/error.writerGuard(andnewsletterAccessGuardfor newsletters, which is itself writer-or-ED) remains authoritative. The shortcut only advertises access.lfx-project-selector(search + All/Foundations/Projects tabs + logos + role badges) via a new curateditemsinput — fed the writer-scoped list, never the view-scoped nav catalog. Sidebar behavior is unchanged whenitemsis absent.Accepted trade-off (persona-differential)
The shortcut is deliberately writer-scoped, so an Executive Director with no per-project
writergrant won't see it (unlikewriterGuard, this has no ED fast-path). EDs still create through the in-page flows, which carry their own ED fast-path. Documented inCreatePermissionService.Test plan
create-quick-link.spec.ts(S1–S5): rail button visibility, six-type grouped popover + render order, dialog + selector (search/tabs), writer-scoped list, and single-project auto-select. Real-API, name-agnostic; skips when the test user holds no writer grant.yarn build/lint/check-typesgreen. (apps/lfx-oneis e2e-only — no unit runner.)Follow-ups (separate tickets)
GET /api/projects/creatable— a dedicated endpoint returning just the writer-creatable set, replacing the current client-triggered full-catalog scan + per-project access-check. Motivation: the rail icon lags on load because the gating call access-checks every visible project.canShowCreateButton()gating call errors — today a transport failure is indistinguishable from "no permission".packages/shared/src/utils/meeting-privacy.utils.spec.ts(describe is not defined, missing vitest import) — unrelated, already failing onmain.🤖 Generated with Claude Code
https://claude.ai/code/session_01UYB2jLJ3sacvXSvQZ6vwCk