Skip to content

[PM-42897] Move the vault banner above the vault header and add to shared folder/my folder pages - #23014

Draft
jengstrom-bw wants to merge 11 commits into
mainfrom
vault/pm-42897/move-the-vault-banner-above-the-vault-header
Draft

[PM-42897] Move the vault banner above the vault header and add to shared folder/my folder pages#23014
jengstrom-bw wants to merge 11 commits into
mainfrom
vault/pm-42897/move-the-vault-banner-above-the-vault-header

Conversation

@jengstrom-bw

Copy link
Copy Markdown
Contributor

🎟️ Tracking

Jira

📔 Objective

📸 Screenshots

@jengstrom-bw jengstrom-bw added t:bugfix Change Type - Bugfix ai-review-vnext Request a Claude code review using the vNext workflow labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This is a layout-only change that hoists vault-organization-user-notifications above the page header on web and desktop, adds it to the My folders and Shared folders pages on both clients, and adds a bit-banner margin/padding correction to the desktop migration stylesheet. Since the last review the only new commits are merges from main; the previously flagged missing tw-shrink-0 on the web Shared folders page was fixed in bf57c68 and that thread is resolved. Re-verified the full diff against the current head — no outstanding code findings.

Code Review Details

No code findings.

Re-verified on the current head:

  • Top-padding rework. Desktop hosts drop tw-py-6; the top gutter comes from bit-header's own tw-pt-8 (VFO1 branch, libs/components/src/header/header.component.html:19) and the side/bottom gutters move onto the inner vault-* element as tw-px-8 tw-pb-6. Every route rendering an affected page is gated on FeatureFlag.VFO1Foundation (apps/desktop/src/app/app-routing.module.ts:492, :514; apps/web/src/app/vault/individual-vault/vault-routing.module.ts:33, :54; apps/web/src/app/oss-routing.module.ts:718), so the non-VFO1 header's -tw-mt-6 — which would have needed the removed tw-pt-6 to cancel — is never rendered on them. The one ungated route, desktop /vault (app-routing.module.ts:467), keeps tw-pt-6 tw-px-6 in its @else branch.
  • migration.scss override scope. bit-layout #main-content bit-banner has specificity 0,1,0,2, which outranks the banner's own arbitrary-variant utilities ([bit-layout_&]:-tw-mx-10, [bit-layout_&]:-tw-mt-6, [bit-layout_&]:tw-pb-6). Desktop instantiates bit-layout once, and vault-organization-user-notifications is the only bit-banner reachable from apps/desktop — the other call sites are browser- and web-only — so the blanket margin: 0; padding-bottom: 0 is contained.
  • tw-shrink-0 placement. Present wherever the banner is a flex item: web and desktop Shared folders (the lib host is tw-flex tw-flex-col tw-h-full tw-min-h-0, libs/vault/src/components/shared-folders/shared-folders.component.ts:165) and desktop vault-v3, whose container is tw-flex tw-flex-col tw-h-full. Correctly omitted on both My folders pages, whose hosts are not flex containers. vault-my-folders declares no host class, so it is display: inline — which is why the desktop page adds tw-block alongside its padding and web, which relies on bit-layout's tw-p-10, does not need to.
  • Web banner bleed. On web, main keeps tw-p-10 with has-[bit-header]:tw-pt-0; the banner's [bit-layout_main:has(bit-header)_&]:tw-mt-0 and [bit-layout_&]:-tw-mx-10 still resolve with the banner as the first child, since :has() matches the nested bit-header regardless of depth.

The failing Rust deny / Rust lint checks touch no files in this diff and appear unrelated.

PR Metadata Assessment

  • QUESTION: This is a purely visual change, but the Objective and Screenshots sections are still empty. Before/after screenshots of the affected pages (web/desktop vault, My folders, Shared folders) would let reviewers confirm the spacing intent.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.57%. Comparing base (7972df6) to head (7551f4a).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...p/src/vault/app/my-folders/my-folders.component.ts 0.00% 1 Missing ⚠️
...b/src/app/vault/my-folders/my-folders.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #23014   +/-   ##
=======================================
  Coverage   55.57%   55.57%           
=======================================
  Files        4412     4412           
  Lines      140969   140969           
  Branches    22392    22392           
=======================================
  Hits        78350    78350           
  Misses      56918    56918           
  Partials     5701     5701           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jengstrom-bw jengstrom-bw added t:feature Change Type - Feature Development and removed t:bugfix Change Type - Bugfix labels Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant