From 11b7aafce1b0dc6edcbed6d4ca885f590b566a65 Mon Sep 17 00:00:00 2001 From: "cb-bot[bot]" Date: Fri, 3 Apr 2026 11:22:24 +0000 Subject: [PATCH] docs(contextbot): update coding conventions Applied 1 convention(s) from recent PR reviews: - AGENTS.md (CREATE): Applied 15 insight(s). --- AGENTS.md | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 1 + 2 files changed, 247 insertions(+) create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..deb29ef --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,246 @@ +# Project Structure + +- **MVVM architecture: Models, Services, ViewModels, Views directory structure.** The project follows a strict MVVM directory structure: Models/ for data types and observable stores, Services/ for stateless logic and I/O abstractions, ViewModels/ for UI state management, and Views/ (with subdirectories for Edit/, Render/, Sidebar/) for SwiftUI views. Windows/ holds AppKit window controllers. + ``` + // Good + Clearance/ + Models/ (RecentFileEntry, DocumentSession, AppSettings) + Services/ (FrontmatterParser, FileIO, AddressBarFormatter) + ViewModels/ (WorkspaceViewModel) + Views/ (WorkspaceView, DocumentSurfaceView) + Windows/ (PopoutWindowController) + + // Bad + Clearance/ + WorkspaceViewModel.swift + WorkspaceView.swift + RecentFileEntry.swift + // all files in flat directory + ``` + +## Dependency Injection & Testability + +- **Dependency injection via default parameters for testability.** Every class that depends on external services (UserDefaults, URLSession, file I/O, panel services) accepts those dependencies through init parameters with production defaults. This allows tests to inject isolated instances without mocks or protocols for simple value-type dependencies. The pattern consistently uses \`= .standard\`, \`= .live\`, \`= .shared\` etc. as default parameter values. + ``` + // Good + init(userDefaults: UserDefaults = .standard, storageKey: String = "recentFiles", maxEntries: Int = 200) { + self.userDefaults = userDefaults + // ... + } + + // Bad + init() { + self.userDefaults = UserDefaults.standard + self.storageKey = "recentFiles" + } + ``` +- **Async closures as injectable dependencies for network operations.** Instead of injecting protocol-based network services, async network operations are injected as closure properties with production defaults. The ViewModel accepts \`@escaping @Sendable (URL) async throws -> RemoteDocument\` with a default that calls the real fetcher. This makes tests trivially substitute synchronous or delayed responses without mock objects. + ``` + // Good + init( + remoteDocumentLoader: @escaping @Sendable (URL) async throws -> RemoteDocument = { requestedURL in + try await RemoteDocumentFetcher.fetch(requestedURL) + } + ) { ... } + + // Bad + protocol RemoteDocumentLoading { + func load(_ url: URL) async throws -> RemoteDocument + } + class MockRemoteDocumentLoader: RemoteDocumentLoading { ... } + ``` +- **Struct-based service types with closure properties for I/O abstraction.** Side-effectful I/O operations are wrapped in lightweight struct types containing closure properties, with a static \`.live\` instance providing the production implementation. This avoids protocol overhead for simple function-bag dependencies while preserving full testability. + ``` + // Good + struct FileIO: Sendable { + var read: @Sendable (URL) throws -> String + var write: @Sendable (String, URL) throws -> Void + + static let live = FileIO( + read: { url in try String(contentsOf: url, encoding: .utf8) }, + write: { text, url in try text.write(to: url, atomically: true, encoding: .utf8) } + ) + } + + // Bad + protocol FileIOProtocol { + func read(url: URL) throws -> String + func write(text: String, url: URL) throws + } + class FileIOImpl: FileIOProtocol { ... } + ``` +- **Protocols only for AppKit boundary types that need full mocking.** Protocols are reserved for types at the AppKit boundary (like NSOpenPanel) where substitution requires more than a closure. Simple dependencies use structs with closure properties or direct injection. This keeps the protocol count low and meaningful. + ``` + // Good + @MainActor + protocol OpenPanelServicing { + func chooseMarkdownFile() -> URL? + } + + struct OpenPanelService: OpenPanelServicing { ... } + + // Bad + protocol FileIOProtocol { ... } + protocol RecentFilesStoring { ... } + // protocols for everything, even simple value-type dependencies + ``` + +## Swift Patterns + +- **Use enum namespaces for stateless service logic.** Pure functions that don't need instance state are grouped as static methods on a caseless enum rather than a struct or class. This signals that the type is a namespace only and cannot be instantiated. + ``` + // Good + enum RemoteDocumentFetcher { + static func resolveForMarkdownRequest(_ requestedURL: URL) -> RemoteDocument { ... } + static func fetch(_ requestedURL: URL, session: URLSession = .shared) async throws -> RemoteDocument { ... } + } + + // Bad + struct RemoteDocumentFetcher { + func resolveForMarkdownRequest(_ requestedURL: URL) -> RemoteDocument { ... } + } + ``` +- **Model types are lightweight structs conforming to standard protocols.** Data models are plain structs that adopt Codable, Equatable, and Identifiable as needed. They use computed properties (not stored) for derived values like displayName or fileURL. No class inheritance or heavyweight patterns. + ``` + // Good + struct RecentFileEntry: Codable, Equatable, Identifiable { + let path: String + var id: String { path } + var displayName: String { + URL(fileURLWithPath: path).lastPathComponent + } + } + + // Bad + class RecentFileEntry: NSObject, NSCoding { + var path: String + var displayName: String + // stored redundantly + } + ``` +- **ObservableObject classes use @Published private(set) for read-only state.** Observable model classes expose state via @Published private(set) properties, forcing mutations to go through dedicated methods. This keeps the mutation API explicit and prevents views from bypassing business logic. + ``` + // Good + final class RecentFilesStore: ObservableObject { + @Published private(set) var entries: [RecentFileEntry] + func add(url: URL) { ... } + } + + // Bad + final class RecentFilesStore: ObservableObject { + @Published var entries: [RecentFileEntry] + // views can mutate directly + } + ``` +- **Guard-early-return pattern for control flow.** Methods consistently use guard-let with early returns rather than nested if-let blocks. This keeps the happy path at the top indentation level and avoids deeply nested conditional logic. + ``` + // Good + guard let url else { + return "" + } + + if url.isFileURL { + return url.path + } + + return simplifiedRemoteText(for: url) + + // Bad + if let url = url { + if url.isFileURL { + return url.path + } else { + return simplifiedRemoteText(for: url) + } + } else { + return "" + } + ``` +- **Generation counter pattern for cancelling stale async operations.** When launching async work that may be superseded by a newer request, increment a generation counter before starting. After the async operation completes, check that the counter hasn't changed before applying results. This avoids complex cancellation token plumbing and ensures only the latest request's result is used. + ``` + // Good + remoteLoadGeneration += 1 + let generation = remoteLoadGeneration + + remoteLoadTask = Task { [weak self] in + let doc = try await self.remoteDocumentLoader(url) + guard self.remoteLoadGeneration == generation, !Task.isCancelled else { return } + self.activeRemoteDocument = doc + } + + // Bad + remoteLoadTask?.cancel() + remoteLoadTask = Task { [weak self] in + let doc = try await self.remoteDocumentLoader(url) + // no staleness check — old task's result could still apply + self.activeRemoteDocument = doc + } + ``` + +## Testing Conventions + +- **Each feature commit includes both implementation and tests.** Feature commits consistently add source files alongside their corresponding test files in the same commit. Tests cover the core contract (happy path, edge cases, round-trip persistence) rather than implementation details. + ``` + // Good + // Single commit adds: + // Clearance/Models/RecentFilesStore.swift + // ClearanceTests/Models/RecentFilesStoreTests.swift + // with tests for add-to-top, dedup, and round-trip persistence + + // Bad + // Commit 1: add RecentFilesStore.swift + // Commit 2 (later): add RecentFilesStoreTests.swift + ``` +- **Descriptive test method names that state the expected behavior.** Test methods follow camelCase naming starting with 'test' followed by a behavioral description: what happens and what is expected. Names describe the scenario, not the implementation detail. No underscores or 'should' phrasing. + ``` + // Good + func testReopeningFileMovesItToTopWithoutDuplicates() + func testLatestRemoteRequestWins() + func testRemoteContentAddsStrictContentSecurityPolicyDirectives() + + // Bad + func test_recent_file_dedup() + func testIt() + func testShouldMoveFileToTop() + ``` +- **Test files mirror source directory structure.** Test files are organized in ClearanceTests/ using the same subdirectory hierarchy as the main source (Models/, Services/, ViewModels/, Edit/, Render/). Test file names match the source file name with a 'Tests' suffix. This makes it trivial to find the corresponding test file for any source file. + ``` + // Good + Clearance/Models/RecentFilesStore.swift -> ClearanceTests/Models/RecentFilesStoreTests.swift + Clearance/Services/RemoteDocumentFetcher.swift -> ClearanceTests/Services/RemoteDocumentFetcherTests.swift + + // Bad + Clearance/Models/RecentFilesStore.swift -> ClearanceTests/RecentFilesStoreTests.swift + // all test files in flat directory + ``` +- **Tests use isolated UserDefaults suites with unique names.** Every test that involves UserDefaults creates a dedicated suite using a unique name (either test-specific or UUID-based), then calls removePersistentDomain to guarantee clean state. This avoids cross-test pollution and allows parallel test execution. + ``` + // Good + let suite = "RecentFilesStoreTests-3" + let defaults = UserDefaults(suiteName: suite)! + defaults.removePersistentDomain(forName: suite) + let store = RecentFilesStore(userDefaults: defaults, storageKey: "recent") + + // Bad + let store = RecentFilesStore() + // relies on shared UserDefaults.standard + ``` +- **Private helper methods in test files for temp file creation.** Test classes define private helper methods like \`makeTempMarkdown(contents:)\` to create temporary test fixtures. These methods use UUID-based directory names for isolation and return URLs that can be passed to the system under test. + ``` + // Good + private func makeTempMarkdown(contents: String) throws -> URL { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: true) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let fileURL = directory.appendingPathComponent("sample.md") + try contents.write(to: fileURL, atomically: true, encoding: .utf8) + return fileURL + } + + // Bad + func testOpenFile() { + let path = "/tmp/test.md" + try! "hello".write(toFile: path, atomically: true, encoding: .utf8) + // uses shared temp path, no cleanup + } + ``` diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md