-
Notifications
You must be signed in to change notification settings - Fork 78
[PM-26809] Prepare Claude configuration #2039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
CLAUDE.md
Outdated
|
||
## Common File Locations | ||
|
||
- Dependency injection: [BitwardenShared/Core/Platform/Services/ServiceContainer.swift](BitwardenShared/Core/Platform/Services/ServiceContainer.swift) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Part of me wonders if this will be sufficient for BWA, or if we need to call BWA out separately, or if we should work at getting some of these files actually migrated to BitwardenKit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rewrite this to take into account both apps. Even though we could move some things into BitwardenKit
there are some objects/services that are particular of each app so we will still need the specific app ServiceContainer
s; so we should still be documenting them.
CLAUDE.md
Outdated
- **Data access**: UI layer mostly uses Repositories (never Stores directly and scarsely Services) | ||
- **State mutations**: Only in Processors | ||
- **Navigation**: Coordinators handle navigation via Routes/Events enums | ||
- **Testing**: Test file goes in same folder as implementation (e.g., `FooProcessor.swift` + `FooProcessorTests.swift`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Should this explicitly call out that we can have multiple test files per class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should. Something like...
- **Testing**: Test file goes in same folder as implementation (e.g., `FooProcessor.swift` + `FooProcessorTests.swift`) | |
- **Testing**: Test files go in the same folder as the implementation (e.g., `FooProcessor.swift` + `FooProcessorTests.swift`). Multiple test files can be used for a single class to improve readability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a direct link to the testing docs explanation on contributing docs as in there, it's explained how test files are organized. Also will make it plural.
Great job! No new security vulnerabilities introduced in this pull request |
CLAUDE.md
Outdated
- **AuthenticatorShared/**: Same as `BitwardenShared` but for Authenticator app. | ||
- **Domains**: Auth | Autofill | Platform | Tools | Vault | ||
- **Apps**: The projects contain two main apps: "Password Manager" and "Authenticator". | ||
- ** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank bullet can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Work in progress need to add some things in there; missed to add them before committing. Thanks for catching this!
CLAUDE.md
Outdated
2. @CONTRIBUTING.md | ||
3. @.github/PULL_REQUEST_TEMPLATE.md | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline can be removed.
CLAUDE.md
Outdated
- **Main Authenticator target**: Authenticator | ||
- **Shared apps frameworks**: BitwardenKit, BitwardenKitMocks, BitwardenResources, AuthenticatorBridgeKit, AuthenticatorBridgeKitMocks, Networking, TestHelpers | ||
|
||
## Critical Rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 ⛈️ I've found terminology like "should not" to be less effective than using more direct phrases like, "You MUST NOT do x
." When possible, providing reasoning is also helpful to express criticality in following instructions.
- You **MUST NOT** install third-party libraries unless explicitly told to.
- New encryption logic **MUST NOT** be added to this repository.
- Before providing any code or making changes, you **MUST** have read and understood the "iOS Client Architecture" and "Code Style" documents listed in the References section.
CLAUDE.md
Outdated
## Testing Requirements | ||
|
||
- Every type with logic needs tests | ||
- Test files named `<TypeToTest>Tests.swift` in same folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we elaborate on this?
- Test files named `<TypeToTest>Tests.swift` in same folder | |
Test files must be named `<TypeToTest>Tests.swift` and be located in the same folder as the file being tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is repeated from the Common patterns
section. I'm wondering if there's actually value in it being in both places. 🤔
CLAUDE.md
Outdated
- Every type with logic needs tests | ||
- Test files named `<TypeToTest>Tests.swift` in same folder | ||
- Snapshot tests use device/iOS version from `.test-simulator-device-name` and `.test-simulator-ios-version` | ||
- Test in light mode, dark mode, and large dynamic type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Can we elaborate more here, too?
- Test in light mode, dark mode, and large dynamic type | |
- Tests must pass in light mode, dark mode, and large dynamic type. |
CLAUDE.md
Outdated
- Test files named `<TypeToTest>Tests.swift` in same folder | ||
- Snapshot tests use device/iOS version from `.test-simulator-device-name` and `.test-simulator-ios-version` | ||
- Test in light mode, dark mode, and large dynamic type | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 It may be helpful to provide instructions on how to run tests.
### How to Run Tests | |
- After making changes, run the tests for the relevant scheme in Xcode to ensure your changes have not broken anything. | |
- You can also run tests from the command line using `xcodebuild`. |
Co-authored-by: Patrick Honkonen <[email protected]>
Co-authored-by: Patrick Honkonen <[email protected]>
Co-authored-by: Patrick Honkonen <[email protected]>
CLAUDE.md
Outdated
@@ -0,0 +1,82 @@ | |||
# Bitwarden iOS - Claude Code Configuration | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to add a clear purpose statement so Claude has immediate context and directive. Something like
This document provides the necessary context and rules for you, an expert iOS developer, to contribute to the Bitwarden iOS project.
## Project Context Files
**Read these files first to fully understand the project and contributing guidelines.**
Files prefixed with `@` are references to other files in this `.ai` directory. You MUST read them for context before proceeding.
The bottom "Files prefixed with @
...", part is to clarify file notation since it varies throughout the doc. Some places, like L7-L9, are prefixed @
while others use hyperlinks pointing to the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll add that. I think I can just switch to use @
in all places as it's the preferred way on Claude docs to avoid stating it here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2039 +/- ##
=======================================
Coverage 85.31% 85.31%
=======================================
Files 1691 1691
Lines 144372 144372
=======================================
+ Hits 123168 123169 +1
+ Misses 21204 21203 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -0,0 +1,348 @@ | |||
# 6502 Kernel Queue System Guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Forgive my ignorance, but what's a 6502 kernel do in the context of iOS development?
🎟️ Tracking
PM-26809
📔 Objective
Prepare
.claude/
configuration files with multiple agents based on https://github.com/dasien/ClaudeMultiAgentTemplate.Also updated
Architecture.md
and addedTesting.md
docs to the repo to add further details to them.⏰ Reminders before review
🦮 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