@@ -77,6 +77,131 @@ The diff shows *what* changed and *how*; a comment exists only to capture *why*
7777- ** No multi-line explanatory blocks, AI-style walkthroughs, or section banners.**
7878- ** Remove a stale comment** only when you're already editing that code for the task at hand — no drive-by cleanups.
7979
80+ ## Coding conventions
81+
82+ These are the points maintainers raise again and again in review. Following them up
83+ front keeps review focused on design instead of the same recurring notes. CodeRabbit
84+ reads this file automatically and reviews against these conventions, so the bot flags
85+ the same things — but the author should not need the bot to learn them.
86+
87+ ### Scope & commits
88+ - ** One concern per PR.** Keep unrelated edits, drive-by refactors, code-style churn,
89+ dependency/lock-file bumps and new runtime-version support out of a feature PR — put
90+ them in a separate PR so they stay backportable to stable branches. Small,
91+ single-purpose PRs sail through; large diffs draw change requests.
92+ - ** The commit type must match the change** (see ` .github/CONTRIBUTING.md ` ): moving code
93+ with no behavior change is ` refactor: ` , not ` feat: ` . Scopes stay broad (` imap ` , ` ui ` ).
94+ - ** Reuse before reinventing.** Grep for an existing helper, mapper, constant or pattern
95+ and mirror it instead of writing a parallel implementation.
96+
97+ ### PHP backend
98+ - ** Types over PHPDoc.** Prefer native param/return/property types; drop PHPDoc that only
99+ restates them; keep ` @throws ` and anything that adds information. Use precise
100+ ` psalm-type ` array shapes (reuse existing shape definitions) instead of bare ` array ` .
101+ Handle Psalm's possible-null and
102+ ` string|false ` (e.g. ` file_get_contents ` ) results — throw a ` ServiceException ` , don't
103+ cast the failure away. Use constructor property promotion and strict comparison
104+ (` === ` , ` in_array(..., true) ` ).
105+ - ** Exceptions.** Catch narrowly — never a blanket ` \Exception ` /` \Throwable ` for a
106+ specific failure. When wrapping, pass the original as ` $previous ` (or rethrow) so the
107+ stack trace survives. Throw domain exceptions; don't leak abstraction-layer exceptions
108+ (` DoesNotExistException ` , storage exceptions) out of a service. Keep ` @throws ` in sync
109+ with the interface contract.
110+ - ** Logging.** Inject ` LoggerInterface ` ; log with a meaningful message and
111+ ` ['exception' => $e] ` context at the right level. Log non-critical conditions at
112+ info/debug and return rather than throwing or spamming warnings. Never swallow errors.
113+ - ** Dependency injection & layering.** Inject collaborators (incl. OCP services) via the
114+ constructor, not the service locator. Replace ` time() ` with ` ITimeFactory ` for
115+ testability. Don't inject request data such as ` userId ` into services (controllers
116+ only) — pass it as an argument so the service stays usable from background jobs. Keep
117+ controllers thin (request/response only); business logic and DB access belong in
118+ services; keep constants in their owning class.
119+ - ** Nextcloud API boundaries.** Never use another app's private ` \OCA\OtherApp\* ` API — go
120+ through a stable OCP interface. Respect the minimum server version in ` appinfo/info.xml ` ;
121+ guard newer OCP APIs with ` method_exists ` + a fallback. Store new config via
122+ ` IAppConfig ` (no dots in new keys). Controllers use ` #[NoAdminRequired] ` for non-admin
123+ routes (omit it on admin-only ones), HTTP 422 for validation errors, ` HTTP::STATUS_* `
124+ constants, and ` TrapError ` over hand-written try/catch. Keep 400 (client error) vs 404
125+ (not found) intentional. Prefer kebab-case URLs with the id in the path and a single
126+ ` resource ` route.
127+ - ** Controller access control.** Check that the current user owns the resource before
128+ acting on an incoming id — guard against IDOR, don't act on a guessed id. Take a nullable
129+ ` ?string $userId ` from the predefined core services and return 401 when it is null.
130+
131+ ### Mappers & entities
132+ - Let mappers propagate ` DoesNotExistException ` / ` OCP\DB\Exception ` to the caller — don't
133+ catch not-found inside the mapper. Reuse the ` QBMapper ` ` insert ` /` update ` /` find ` helpers
134+ instead of hand-writing queries.
135+ - Name accessors ` findX ` /` getX ` (not ` store ` ); keep entity ` @method ` annotations accurate,
136+ including ` int|null ` for nullable columns; register column types with ` addType ` in the
137+ constructor.
138+
139+ ### Database & migrations
140+ - ** Re-runnable:** check column/table existence before changing schema so a partially
141+ failed migration can retry.
142+ - ** Version naming:** name the ` Version ` class after the * next unreleased* minor and bump
143+ ` version ` in ` appinfo/info.xml ` , or the migration won't run. (See the DB index dual
144+ pattern for adding indices without a blocking ` changeSchema ` .)
145+ - ** Foreign keys** on referencing columns, with the delete action chosen from the
146+ relationship: cascade delete for rows the parent owns (so account/mailbox deletion leaves
147+ no orphans), ` SET NULL ` for an optional reference, ` RESTRICT ` to prevent deletion. Clean up
148+ dangling rows pre-schema.
149+ - ** Indexes:** composite column order matters (` [a,b] ` ≠ ` [b,a] ` ) — match the query's
150+ WHERE/ORDER BY; add covering indexes on exactly the filtered/joined columns; make the
151+ index unique when the column is.
152+ - ** Portability & size:** sensible column lengths (avoid MariaDB off-row storage), truncate
153+ before writing fixed-width columns, Oracle needs nullable booleans and treats empty
154+ strings as ` NULL ` , Postgres/Oracle are strict about VARCHAR-vs-INT. Batch huge
155+ UPDATE/DELETE and emit progress. Inline entity constants in migrations (a loaded class
156+ can hold stale values mid-upgrade).
157+
158+ ### Performance
159+ - Push filters/limits/cursors into the DB query; scope by ` user_id ` ; use ` WHERE ... IN `
160+ to avoid N+1; don't ` array_merge ` in a loop. Reuse one Horde IMAP client across a bulk
161+ operation instead of reconnecting per message. Stream large result sets via generators
162+ and guard against OOM — users can have 200+ mailboxes. Prefer a local cache over a
163+ distributed one for recomputable values and scope cache keys per user.
164+
165+ ### Frontend (Vue / JS)
166+ - ** Async:** ` async ` /` await ` with ` try ` /` catch ` , never mixed with ` .then ` ; never ` await `
167+ inside ` forEach ` (use ` for...of ` ); a missing ` await ` is a real bug. Await sequential
168+ per-item dispatches instead of flooding the backend, and don't fire one request per
169+ list item — push it to the backend or preload via initial state.
170+ - ** Structure:** HTTP handling in the service layer, mutations inside store actions,
171+ business logic out of components. Add a loading/disabled state to any control that
172+ triggers an async action so a double click can't fire it twice. Don't make an element
173+ look clickable when its action is unavailable.
174+ - ** Style of code:** early returns over nested conditions, named constants over magic
175+ numbers, ` const ` /` let ` never ` var ` , pure helpers free of side effects and store access.
176+ Sanitize user-controlled values before they reach the DOM. Use ` isDarkTheme ` from
177+ ` @nextcloud/vue ` , not ` window.matchMedia('(prefers-color-scheme: dark)') ` . Follow the
178+ dev-manual naming the linter can't check: multi-word PascalCase component names
179+ (` SettingsView ` , not ` Settings ` ), prefixed sub-components, and acronyms with only the
180+ first letter capitalised (` callHttpApi ` ).
181+ - ** CSS:** see [ Styling] ( #styling ) ; keep styles scoped, follow BEM, prefer a modifier class
182+ over manipulating inline style, use grid/spacing/breakpoint CSS variables (no hard-coded
183+ breakpoints), and remove now-unused styles. Avoid ` ::v-deep ` /` !important ` into upstream
184+ component internals; where a deep selector is genuinely needed, comment why so it isn't
185+ dropped by mistake.
186+
187+ ### Internationalization
188+ - Wrap every user-facing string, ** including aria-labels** , in ` t('mail', …) ` . Use one
189+ string with placeholders — never concatenate translated fragments (translators reorder
190+ words). Use ` n('mail', …) ` with a ` %n ` placeholder for counts. Never compare against a
191+ hard-coded English string or use a translated string as a key. No HTML inside a
192+ translation string — translate the plain parts and HTML-encode the inserts.
193+
194+ ### Accessibility
195+ - Real anchor (` <a href … target="_blank"> ` ) for links, not a JS click handler, so screen
196+ readers and native middle-click work. Use semantically correct elements and let
197+ ` NcButton ` inject the required a11y attributes rather than hand-rolling clickable markup.
198+
199+ ### Mail-specific gotchas
200+ - ** IMAP UIDs are only unique within one mailbox** — never treat them as global
201+ identifiers; key on the database primary key.
202+ - The app already has building blocks (trusted senders, RFC-2822 address parsing,
203+ ` IMAPClientFactory ` , ` Horde_Mail_Rfc822_Identification ` ) — reuse them.
204+
80205## Testing
81206
82207### Unit Tests
@@ -86,6 +211,11 @@ Located in `tests/Unit/` with structure mirroring `lib/`.
86211- Use ** arrange-act-assert** structure with blank lines separating each phase (no literal comments)
87212- Mock dependencies via ` $this->createMock(Interface::class) `
88213- Setup mocks in ` setUp() ` for common fixtures
214+ - ** Cover the error and edge paths** , not just the happy path (empty input, the throwing branch, both sort orders); new classes and changed logic need tests
215+ - Declare typed fixture properties (` private Foo&MockObject $foo; ` ) to avoid dynamic-property deprecation warnings; test methods return ` void `
216+ - Only mock external collaborators, never the class under test; assert arguments with ` ->with(...) ` and, instead of the removed ` withConsecutive ` , branch on the argument with ` match ` /` if ` so behavior depends on input, not call order
217+ - Use the same constants in tests as in production code (don't hard-code their values)
218+ - Hand-written stubs of another app's API give false safety — Psalm keeps passing when the upstream API changes, so keep them in sync or avoid them
89219
90220#### Running Tests
91221``` bash
0 commit comments