0028 - Adopt @ngrx/signals for Component State Stores

[Banrion]

📔 Objective

This pull request adds ADR 0028 - Adopt @ngrx/signals for Component State Stores to expose the ADR to a greater audience and receive feedback.

Note: This pull request also adds a placeholder file for the 0027 ADR to Adopt Angular Signals for component state that this ADR builds upon.

⏰ 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

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	58effff
Status:	🚫  Build failed.

View logs

[github-actions]

Checkmarx One – Scan Summary & Details – 17b29f96-f4c9-46d6-aa58-e75abc656f1f

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

[withinfocus]

❓ Did lint need this? Your only usage is in a link which wasn't checked I thought.

We keep this alphabetically sorted by the way.

[Banrion]

I did, it's specifically the cspell check that causes the failure. Thanks for catching that it wasn't sorted!

[withinfocus]

Suggested change

	status: "Draft"
	status: "Proposed"

[withinfocus]

❓ What's happening with this whole file? Not understanding. You can have one proposed ADR as 27 most likely.

[Banrion]

It was a placeholder for the 0027 - Angular signals ADR. Now that there is an active pull request, #638, I can remove the file

[withinfocus]

🎨 Can your code reference be simplified somehow? Maybe just say "Angular Signals"? A lot of title usages can't render the preformatted text, but you can use the code in paragraphs.

[withinfocus]

❌ This document should be retired in favor of the ADR, and therefore won't be linkable.

[withinfocus]

❌ These are also not available to the community, so content should all be here.

[Banrion]

Thanks, I've updated the links now that the Draft ADR references is in a pull request.

[withinfocus]

Suggested change

	**Adopt @ngrx/signals for state management on the component level**
	Chosen option: **Adopt @ngrx/signals for state management on the component level.**

[Hinton]

If we're not storing decrypted data. What would we be using it for? Purely concerns like "is drawer open"?

[Banrion]

Good question! We should still store the encrypted data in the stores. There are a few methods we can use to expose the decrypted data while it's in flight. When we start the migrations, we should and plan to explore the different methods to determine which best suites our security policies. This statement is to cover the overall idea that encrypted data is encrypted at rest. The main two methods to explore are using signal store computed signals, could be considered in flight but these are still exposed from the store, or expose a feature to signal stores that provides the functions to encrypt and decrypt within the component itself. The main difference is where we want the responsibility of encrypting and decrypting to lie architecturally.

[claude]

Claude finished @Banrion's task —— View job

---

PR Review: ADR 0028 - Adopt @ngrx/signals for Component State Stores

I've completed my review of this ADR documentation. Here are my findings and recommendations:

Overall Assessment

This is a well-structured ADR that follows the established repository patterns and provides clear technical reasoning for adopting @ngrx/signals. The document effectively builds upon ADR 0027 and presents a compelling case for standardized component state management.

Detailed Review Feedback

✅ Strengths

1. Strong Technical Foundation: The ADR clearly articulates the problem and builds logically on ADR 0027
2. Well-Structured Content: Follows the established ADR format with appropriate sections
3. Comprehensive Analysis: Covers positive/negative consequences and mitigation strategies
4. Practical Guidelines: Provides specific guidance on when to use signal stores and security considerations
5. Good References: Includes relevant external resources and proof-of-concept links

⚠️ Issues to Address

High Priority

1. File Naming Convention (docs/architecture/adr/0028-ngrx-signals.md:1)

   • Issue: Filename doesn't follow the established convention (####-title.md)
   • Current: 0028-ngrx-signals.md
   • Expected: 0028-adopt-ngrx-signals-for-component-state-stores.md
   • Impact: Inconsistent with repository naming standards seen in other ADRs

2. Broken Internal Reference (docs/architecture/adr/0028-ngrx-signals.md:14)

   • Issue: Reference to ./0027-angular-signals.md - this file doesn't exist in the codebase
   • Current: [ADR 0027 - Adopt Angular Signals for Component State](./0027-angular-signals.md)
   • Impact: Dead link that will break navigation and user experience

3. External Link Dependency (docs/architecture/adr/0028-ngrx-signals.md:106-109)

   • Issue: Links to external Google resources that may become inaccessible to community users
   • Concern: Goes against repository guidelines for content accessibility
   • Recommendation: Either embed key content or provide alternative public resources

Medium Priority

4. Title Formatting (docs/architecture/adr/0028-ngrx-signals.md:8)

   • Issue: Uses backticks in title which may not render properly in all contexts
   • Current: # 0028 - Adopt \@ngrx/signals` for Component State Stores`
   • Recommendation: Consider simplifying to # 0028 - Adopt @ngrx/signals for Component State Stores

5. Word Ordering in Custom Dictionary (custom-words.txt:45)

   • Issue: "NGRX" should be alphabetically ordered before "OIDCS"
   • Impact: Violates repository's alphabetical sorting requirement

Low Priority - Style & Clarity

6. Grammar Issues:

   • Line 36: "that complements" should be "that complement"
   • Line 64: "Builds existing signal adoption" should be "Builds on existing signal adoption"

7. Repetitive Language (docs/architecture/adr/0028-ngrx-signals.md:61-62):

   • "Captures repeating patterns (huge bonus)" - the parenthetical adds unnecessary informality

🎨 Suggestions for Improvement

1. Enhanced Security Section: Consider expanding the security guidelines with specific examples of what constitutes "sensitive data" in Bitwarden's context

2. Implementation Timeline: The "Plan" section could benefit from more specific timelines or milestones

3. Migration Strategy: More detailed guidance on migrating existing component state would be valuable

✅ What's Working Well

• Proper frontmatter structure with all required fields
• Appropriate use of AdrTable component
• Clear problem statement and context
• Balanced consideration of alternatives
• Practical guidelines section
• Good use of repository conventions for most formatting

Recommendation

CHANGES REQUESTED - The broken internal link and external dependencies need to be resolved before merge. The filename convention should also be addressed for consistency.

Priority Actions:

1. Fix the broken reference to ADR 0027
2. Address external link accessibility concerns
3. Correct filename to follow naming convention
4. Fix alphabetical ordering in custom-words.txt

Once these issues are addressed, this will be a solid addition to the architecture documentation that provides clear guidance for the development team.