[PM-33572] llm: Add implementing-ios-code skill with Swift templates#2446
[PM-33572] llm: Add implementing-ios-code skill with Swift templates#2446SaintPatrck wants to merge 2 commits intomainfrom
Conversation
github-actions
bot
added
the
t:llm
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Claude finished @SaintPatrck's task in 7m 27s —— View job I'll analyze this and get back to you. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2446 +/- ##
==========================================
- Coverage 87.09% 87.08% -0.01%
==========================================
Files 1859 1859
Lines 164348 164348
==========================================
- Hits 143132 143127 -5
- Misses 21216 21221 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Overall Assessment: APPROVE Reviewed 2 new files adding an LLM skill for implementing Bitwarden iOS features with step-by-step workflow and code templates. Code Review Details
|
Inline FindingsFinding 1 -
|
- SKILL.md: step-by-step implementation workflow (Core layer first, then UI file-set, DI wiring, security checklist, documentation); prerequisites check for design doc and Docs/Architecture.md - templates.md: minimal copy-paste skeletons derived from actual codebase files — Coordinator, Processor, State/Action/Effect, View, Service, Repository; each template references the source file it was based on
SaintPatrck
force-pushed
the
llm/implementing-ios-code
branch
from
6f815a6 to
8668f72
Compare
Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed 2 new files adding an LLM skill for implementing Bitwarden iOS features. The skill provides a step-by-step workflow (SKILL.md) and copy-paste code templates (templates.md) for Coordinator, Processor, State/Action/Effect, View, Service, and Repository patterns. Templates were verified against actual codebase files and correctly reflect current conventions, including the weak optional Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| From the plan, identify: | ||
| - Is this a new feature (full file-set) or modification of existing code? | ||
| - Which framework: `BitwardenShared`, `AuthenticatorShared`, or `BitwardenKit`? | ||
| - Which domain: `Auth/`, `Autofill/`, `Platform/`, `Tools/`, `Vault/`? |
There was a problem hiding this comment.
Billing/
Details and fix
CLAUDE.md defines six fixed subdirectories for Core/ and UI/: Auth/, Autofill/, Billing/, Platform/, Tools/, Vault/. This list omits Billing/, which could lead the LLM to place billing-related code in the wrong domain directory.
| - Which domain: `Auth/`, `Autofill/`, `Platform/`, `Tools/`, `Vault/`? | |
| - Which domain: `Auth/`, `Autofill/`, `Billing/`, `Platform/`, `Tools/`, `Vault/`? |
| **Data Models** (if needed) | ||
| - Request/Response types in `Core/<Domain>/Models/Request/` and `Response/` | ||
| - Enum types in `Core/<Domain>/Models/Enum/` | ||
| - CoreData entities only if persistence is needed (add to `Bitwarden.xcdatamodeld`) |
There was a problem hiding this comment.
🤔 We basically never add anything to Core Data. If something's stored on disk, it's either in UserDefaults or the Keychain, depending on how sensitive of data it is.
Maybe this should be
- More-sensitive information persisted to disk is in `KeychainRepository`. Less-sensitive information persisted to disk is in `AppSettingsStore`. Both of these are exposed through `StateService`. Try to use a separate protocol instead of adding to the `StateService, `AppSettingsStore`, and `KeychainRepository` protocols, to ensure interface segregation.
|
|
||
| Before finishing: | ||
| - [ ] Vault data? → Must use `BitwardenSdk` for all encryption/decryption | ||
| - [ ] Storing credentials? → Must use `KeychainRepository`, not `UserDefaults` |
There was a problem hiding this comment.
| - [ ] Storing credentials? → Must use `KeychainRepository`, not `UserDefaults` | |
| - [ ] Storing credentials? → Must use `KeychainRepository`, not `AppSettingsStore` |
|
|
||
| All new public types and methods require DocC (`///`) documentation. | ||
| Exceptions: protocol property/function implementations (docs live in the protocol), mock classes. | ||
| Use `// MARK: -` sections to organize code within files. |
There was a problem hiding this comment.
| Use `// MARK: -` sections to organize code within files. | |
| Use pragma marks to organize code. `// MARK: -` is used to denote different objects in the same file; `// MARK:` is used to denote different sections within an object. |
| When adding a new screen, create these files (replace `<Feature>` throughout): | ||
|
|
||
| ``` | ||
| BitwardenShared/UI/<Domain>/<Feature>/ |
There was a problem hiding this comment.
🤔 Sometimes a new screen requires a coordinator, but not always; often one coordinator manages multiple screens (and therefore multiple processors). I'm not sure how best to capture that judgement call here.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33572
📔 Objective
Adds a Claude skill for implementing Bitwarden iOS features following established patterns.
What changed:
skills/implementing-ios-code/SKILL.md— Step-by-step implementation workflow: determines scope from a design doc, builds the Core layer first (models, services, repositories), then the full UI file-set (Coordinator, Processor, State, Action, Effect, View), wires DI intoServiceContainer, runs a security checklist (zero-knowledge, Keychain, InputValidator, NonLoggableError, extension memory), and ensures DocC documentation. ReferencesDocs/Architecture.mdas authoritative and does not duplicate it.skills/implementing-ios-code/templates.md— Minimal copy-paste skeletons derived from actual codebase files: Coordinator, Processor, State/Action/Effect, View (withstore.bindingandstore.perform), Service (protocol +Default*+Has*+// sourcery: AutoMockable), and Repository. Each template notes the source file it was based on.