Skip to content

Conversation

@addisonbeck
Copy link
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24802

📔 Objective

Encourage using Nx commands for local builds of apps. When the npm build option is removed we'll come back around and update this again.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@addisonbeck addisonbeck requested a review from a team as a code owner October 14, 2025 15:57
@claude
Copy link

claude bot commented Oct 14, 2025

Claude finished @addisonbeck's task —— View job


Review Complete ✅

I've thoroughly reviewed this PR that adds callouts encouraging the use of Nx commands for client builds across 4 documentation files.

Tasks:

  • Read changed files to understand the modifications
  • Analyze the added callouts for clarity and accuracy
  • Check adherence to repository documentation guidelines
  • Review grammar, spelling, and formatting
  • Verify code examples follow best practices
  • Provide comprehensive review feedback

Review Feedback

Positive Aspects

  1. Consistent implementation: All 4 client documentation files (browser, CLI, desktop, web-vault) receive the same treatment with similar callout structure.

  2. Good placement: The Nx callouts are positioned correctly at the beginning of the "Build instructions" sections, making them highly visible.

  3. Proper use of Docusaurus syntax: The :::tip callouts follow established Docusaurus patterns used throughout the documentation.

  4. Comprehensive examples: Each callout includes relevant configuration options (e.g., --configuration=firefox-dev, --configuration=oss-dev).

  5. Helpful reference: All callouts consistently link to the complete Nx documentation.

🎨 Suggestions for Improvement

  1. Inconsistent callout titles: The callout titles vary slightly across files:

    • Browser: "Nx commands are preferred We now recommend..."
    • CLI: "Nx commands are preferred We now recommend..."
    • Desktop: "Nx commands are preferred We now recommend..."
    • Web: "Nx commands are preferred We now recommend..."

    Issue: Missing punctuation after "preferred" makes the title read awkwardly. Consider either:

    • "Nx commands are preferred. We now recommend..." (with period)
    • "Nx commands are now preferred" (simplified)
    • "Use Nx commands (preferred)" (even more concise)
  2. Code comment consistency: The CLI file has slightly different comment style:

    # Build and watch (GPL)
    # Build and watch (Bitwarden)

    While others use:

    # Build and watch (chrome is the default)
    # Build and serve (including the native module)

    Consider standardizing the comment format across all files.

  3. Desktop-specific consideration: The desktop callout mentions "Build and serve (including the native module)" which is helpful context, but the existing documentation already covers native module building. This is fine, but ensure the flow between the Nx command and native module instructions is clear.

Documentation Standards Compliance

  • Style guide adherence: ✅ Uses code blocks properly, maintains brevity
  • Consistent formatting: ✅ Follows established callout patterns
  • Code examples: ✅ All commands are syntactically correct
  • Frontmatter preservation: ✅ No changes to existing frontmatter
  • Conditional content: ✅ Appropriately respects existing <Community> and <Bitwarden> tags

⚠️ Minor Issues

  1. Grammar: The title text flows awkwardly without punctuation between "preferred" and "We"
  2. Link target: The referenced Nx documentation link https://github.com/bitwarden/clients/blob/main/docs/using-nx-to-build-projects.md should be verified to exist (unable to verify externally)

📝 Technical Accuracy

The Nx commands appear technically sound:

  • npx nx serve [project] is the correct Nx syntax
  • Configuration flags like --configuration=oss-dev follow Nx patterns
  • Project names (browser, cli, desktop, web) align with the documentation structure

🎯 Overall Assessment

This is a well-executed documentation update that successfully achieves its objective of encouraging Nx command usage. The changes are consistent, well-placed, and technically accurate. The only significant issue is the minor grammatical awkwardness in the callout titles that could be easily addressed.

Recommendation: ✅ Approve with minor suggestions - The changes effectively serve their purpose and follow documentation standards. Consider addressing the title punctuation for improved readability.


@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 14, 2025

Deploying contributing-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7c14d6a
Status: ✅  Deploy successful!
Preview URL: https://1c207216.contributing-docs.pages.dev
Branch Preview URL: https://nx-docs.contributing-docs.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Oct 14, 2025

Logo
Checkmarx One – Scan Summary & Details0ff37ae2-1c15-442f-8f9f-8114fed5e580

Great job! No new security vulnerabilities introduced in this pull request

@claude
Copy link

claude bot commented Oct 14, 2025

Claude finished @addisonbeck's task —— View job


Review Complete ✅

I've thoroughly reviewed this PR that adds callouts encouraging the use of Nx commands for client builds across 4 documentation files.

Tasks:

  • Read changed files to understand the modifications
  • Analyze the added callouts for clarity and accuracy
  • Check adherence to repository documentation guidelines
  • Review grammar, spelling, and formatting
  • Verify code examples follow best practices
  • Provide comprehensive review feedback

Review Feedback

Positive Aspects

  1. Consistent implementation: All 4 client documentation files (browser, CLI, desktop, web-vault) receive the same treatment with similar callout structure.

  2. Good placement: The Nx callouts are positioned correctly at the beginning of the "Build instructions" sections, making them highly visible.

  3. Proper use of Docusaurus syntax: The :::tip callouts follow established Docusaurus patterns used throughout the documentation.

  4. Comprehensive examples: Each callout includes relevant configuration options (e.g., --configuration=firefox-dev, --configuration=oss-dev).

  5. Helpful reference: All callouts consistently link to the complete Nx documentation.

🎨 Suggestions for Improvement

  1. Grammar issue in callout titles: The callout titles read awkwardly: "Nx commands are preferred We now recommend..."

    Issue: Missing punctuation after "preferred" makes the title run into the next sentence. Consider:

    • "Nx commands are preferred. We now recommend..." (with period)
    • "Nx commands are now preferred" (simplified)
    • "Use Nx commands (preferred)" (even more concise)
  2. Code comment consistency: The CLI file has slightly different comment style:

    # Build and watch (GPL)
    # Build and watch (Bitwarden)

    While others use:

    # Build and watch (chrome is the default)
    # Build and serve (including the native module)

    Consider standardizing the comment format across all files.

  3. Desktop-specific consideration: The desktop callout mentions "Build and serve (including the native module)" which is helpful context, but ensure the flow between the Nx command and existing native module instructions is clear.

Documentation Standards Compliance

  • Style guide adherence: ✅ Uses code blocks properly, maintains brevity
  • Consistent formatting: ✅ Follows established callout patterns
  • Code examples: ✅ All commands are syntactically correct
  • Frontmatter preservation: ✅ No changes to existing frontmatter
  • Conditional content: ✅ Appropriately respects existing <Community> and <Bitwarden> tags

⚠️ Minor Issues

  1. Grammar: The title text flows awkwardly without punctuation between "preferred" and "We"
  2. Link target: The referenced Nx documentation link https://github.com/bitwarden/clients/blob/main/docs/using-nx-to-build-projects.md should be verified to exist (unable to verify externally)

📝 Technical Accuracy

The Nx commands appear technically sound:

  • npx nx serve [project] is the correct Nx syntax
  • Configuration flags like --configuration=oss-dev follow Nx patterns
  • Project names (browser, cli, desktop, web) align with the documentation structure

🎯 Overall Assessment

This is a well-executed documentation update that successfully achieves its objective of encouraging Nx command usage. The changes are consistent, well-placed, and technically accurate. The only significant issue is the minor grammatical awkwardness in the callout titles that could be easily addressed.

Recommendation: ✅ Approve with minor suggestions - The changes effectively serve their purpose and follow documentation standards. Consider addressing the title punctuation for improved readability.


Copy link

@willmartian willmartian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@addisonbeck addisonbeck merged commit 0321f60 into main Oct 14, 2025
14 checks passed
@addisonbeck addisonbeck deleted the nx-docs branch October 14, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants