feat: add theme registry distribution#319
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full theming system: resilient ThemeDefinition models, ThemeStorage persistence, ThemeRegistryInstaller for ZIP-based registry themes, ThemeEngine singleton for lifecycle and application, SwiftUI theme editor/list views, plugin install integration for themes, and extensive documentation and changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Browse as BrowsePluginsView
participant Installer as ThemeRegistryInstaller
participant Network as Network
participant Storage as ThemeStorage
participant Engine as ThemeEngine
User->>Browse: Tap "Install Theme"
Browse->>Installer: install(plugin, progress)
Installer->>Installer: Validate manifest & app version
Installer->>Network: Download ZIP
Network-->>Installer: ZIP bytes
Installer->>Installer: Verify SHA-256 & extract JSON
Installer->>Installer: Decode ThemeDefinition(s), rewrite IDs
Installer->>Storage: saveRegistryTheme(...)
Storage-->>Installer: persisted
Installer->>Storage: saveRegistryMeta(...)
Storage-->>Installer: meta persisted
Installer->>Engine: reloadAvailableThemes()
Engine-->>Installer: reload complete
Installer-->>Browse: Success
Browse->>User: Installation complete (UI update)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
TablePro/Theme/RegistryThemeMeta.swift (1)
3-16: Add explicit access control to types.Both
RegistryThemeMetaandInstalledRegistryThemelack explicit access control modifiers. As per coding guidelines, always use explicit access control on types.Proposed fix
-struct RegistryThemeMeta: Codable { +internal struct RegistryThemeMeta: Codable { var installed: [InstalledRegistryTheme] init(installed: [InstalledRegistryTheme] = []) { self.installed = installed } } -struct InstalledRegistryTheme: Codable, Identifiable { +internal struct InstalledRegistryTheme: Codable, Identifiable { let id: String let registryPluginId: String let version: String let installedDate: Date }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/RegistryThemeMeta.swift` around lines 3 - 16, Add explicit access control modifiers to the types and their members: make the structs and their properties public (or internal/private as appropriate for your module) by adding e.g. "public" to the declarations of RegistryThemeMeta and InstalledRegistryTheme and to their stored properties and initializer (init) so access intent is explicit; ensure Identifiable conformance and Codable conformance remain unchanged while applying the chosen access level to id, registryPluginId, version, installedDate and the installed property and initializer of RegistryThemeMeta.CHANGELOG.md (1)
10-12: Consider consolidating the duplicate "Added" sections.There are now two
### Addedsections under[Unreleased](lines 10 and 36). Per Keep a Changelog format, entries of the same type should be grouped under a single section header. Consider moving this entry to the existing### Addedsection at line 36.Suggested consolidation
Remove lines 10-12 and add the entry to the existing Added section:
### Added +- Theme registry distribution: browse, install, uninstall, and update community themes from the plugin registry (Settings > Plugins > Browse, filtered by Themes category) - Full theme engine with 9 built-in presets (Default Light/Dark, Dracula, Solarized Light/Dark, One Dark, GitHub Light/Dark, Nord) and custom theme support🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 10 - 12, There are two duplicate "### Added" headers; remove the top one that contains the entry "Theme registry distribution: browse, install, uninstall, and update community themes from the plugin registry (Settings > Plugins > Browse, filtered by Themes category)" and move that bullet into the existing "### Added" section elsewhere under [Unreleased] so all Added items are consolidated under a single "### Added" header.TablePro/Views/Settings/Appearance/ThemeListRowView.swift (1)
3-4: Add explicit access control to the struct.
ThemeListRowViewlacks an explicit access control modifier. As per coding guidelines, always use explicit access control on types.Proposed fix
-struct ThemeListRowView: View { +internal struct ThemeListRowView: View { let theme: ThemeDefinition🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/Appearance/ThemeListRowView.swift` around lines 3 - 4, ThemeListRowView currently has no explicit access control; add the appropriate access modifier to the struct declaration (e.g., public or internal) to match the module's API surface and coding guidelines. Locate the ThemeListRowView struct and prepend the chosen access level to its declaration, and ensure any related symbol usages (initializers or consumers of ThemeListRowView) remain accessible under the new modifier.TablePro/Views/Settings/Appearance/ThemeEditorView.swift (2)
34-37: Tab labels use raw enum values which may not localize correctly.
Text(tab.rawValue)displays the English raw values ("Fonts", "Colors", "Layout") directly. While these happen to be English words, they won't be localized for other languages. Consider usingString(localized:)for proper localization.Suggested approach
private enum EditorTab: String, CaseIterable { case fonts = "Fonts" case colors = "Colors" case layout = "Layout" + + var localizedName: String { + switch self { + case .fonts: return String(localized: "Fonts") + case .colors: return String(localized: "Colors") + case .layout: return String(localized: "Layout") + } + } }Then use
Text(tab.localizedName)in the Picker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/Appearance/ThemeEditorView.swift` around lines 34 - 37, Picker is currently using Text(tab.rawValue) which shows the enum's English raw values and won't localize; update the EditorTab enum to provide a localizedName computed property (e.g., var localizedName: String { String(localized: ...) }) and change the Picker to use Text(tab.localizedName) so labels are localized; ensure EditorTab conforms to CaseIterable and use the new localizedName wherever tab display strings are needed.
10-10: Add explicit access control to the struct.Per coding guidelines, always use explicit access control on types.
Suggested fix
-struct ThemeEditorView: View { +internal struct ThemeEditorView: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/Appearance/ThemeEditorView.swift` at line 10, Add an explicit access control modifier to the ThemeEditorView type declaration (replace the implicit default on struct ThemeEditorView: View). Update the declaration to include the intended visibility (for example "internal struct ThemeEditorView: View" if it is module-internal, or "public struct ThemeEditorView: View" if it is part of the module API) so the type follows the project's explicit access-control guideline.TablePro/Views/Settings/Appearance/ThemeListView.swift (2)
143-154: Silent error handling may hide failures from users.The
try?pattern silently discards errors induplicateActiveTheme(),deleteSelectedTheme(), and similar methods. While the UI will still update, users won't know if their theme failed to save or delete. Consider logging failures or showing feedback for critical operations.Suggested improvement for save/delete operations
private func duplicateActiveTheme() { let theme = engine.activeTheme let copy = engine.duplicateTheme(theme, newName: theme.name + " (Copy)") - try? engine.saveUserTheme(copy) + do { + try engine.saveUserTheme(copy) + } catch { + // Consider logging or showing error feedback + return + } engine.activateTheme(copy) selectedThemeId = copy.id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/Appearance/ThemeListView.swift` around lines 143 - 154, The methods duplicateActiveTheme and deleteSelectedTheme currently swallow errors via try?; change them to use do-catch so failures from engine.saveUserTheme and engine.deleteUserTheme are surfaced — in the catch block log the error (via your logger) and present user-facing feedback (e.g., an alert/toast) and avoid updating state like selectedThemeId or calling engine.activateTheme unless the operation succeeded; ensure the code references engine.duplicateTheme, engine.saveUserTheme, engine.activateTheme, engine.deleteUserTheme and selectedThemeId so the UI only updates after a successful save/delete and errors are logged and shown to the user.
5-5: Add explicit access control to the struct.Per coding guidelines, always use explicit access control on types. The struct should have an explicit
internal(or appropriate) modifier.Suggested fix
-struct ThemeListView: View { +internal struct ThemeListView: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Settings/Appearance/ThemeListView.swift` at line 5, The ThemeListView struct lacks an explicit access-control modifier; update the declaration of ThemeListView to include an explicit modifier (e.g., prepend internal or the appropriate visibility such as public) so the type conforms to project guidelines—locate the struct declaration "ThemeListView: View" and add the chosen access level before "struct ThemeListView".TablePro/Theme/ThemeDefinition.swift (1)
3-3: Add explicit access control to types.Per coding guidelines, types should have explicit access control. This applies to
ThemeDefinition,ThemeAppearance,SyntaxColors,EditorThemeColors, and all other structs/enums in this file.Example for main struct
-struct ThemeDefinition: Codable, Identifiable, Equatable, Sendable { +internal struct ThemeDefinition: Codable, Identifiable, Equatable, Sendable {Apply similar changes to all type declarations in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeDefinition.swift` at line 3, Add explicit access control modifiers to every type in this file (replace implicit defaults) by prepending the appropriate access level (e.g., public or internal per project guidelines) to the top-level declarations such as ThemeDefinition, ThemeAppearance, SyntaxColors, EditorThemeColors and any other structs/enums in the file; be sure to apply the same explicit access modifier to nested types and related typealiases so all type declarations are explicit and consistent with the module's intended visibility.TablePro/Theme/ThemeRegistryInstaller.swift (2)
15-15: Add explicit access control to the class.Per coding guidelines, always use explicit access control on types.
Suggested fix
-final class ThemeRegistryInstaller { +internal final class ThemeRegistryInstaller {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` at line 15, The ThemeRegistryInstaller type lacks explicit access control; update the declaration of ThemeRegistryInstaller to include an explicit modifier (e.g., make ThemeRegistryInstaller either public final class ThemeRegistryInstaller or internal final class ThemeRegistryInstaller) based on its intended visibility, and ensure any related symbols (constructors or usages) are adjusted if you change visibility.
194-198: Version comparison uses string inequality instead of semantic versioning.
plugin.version != installedcompares version strings directly. This won't correctly identify updates when versions differ semantically (e.g., "1.0.0" vs "1.0.1"). Consider using semantic version comparison.Suggested approach
return manifest.plugins.filter { plugin in guard plugin.category == .theme, let installed = installedVersions[plugin.id] else { return false } - return plugin.version != installed + return plugin.version.compare(installed, options: .numeric) == .orderedDescending }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 194 - 198, The filter currently compares versions with string inequality (plugin.version != installed); change the comparison inside the manifest.plugins filter closure to parse plugin.version and installedVersions[plugin.id] into semantic-version objects (using your project's SemVer/Version type or a lightweight parser) and compare them semantically (e.g., Version(plugin.version) != Version(installed)). Update the filter logic in ThemeRegistryInstaller where manifest.plugins is filtered so that unparsable versions are handled safely (fallback to string compare or treat as unequal) and ensure you reference plugin.version and installedVersions[plugin.id] when creating the Version instances.TablePro/Theme/ThemeEngine.swift (1)
73-73: Add explicit access control to the class and helper structs.Per coding guidelines, types should have explicit access control. This applies to
ThemeEngine,EditorFontCache,DataGridFontCacheResolved, andDataGridFontVariant.Examples
-final class ThemeEngine { +internal final class ThemeEngine { -struct EditorFontCache { +internal struct EditorFontCache { -struct DataGridFontCacheResolved { +internal struct DataGridFontCacheResolved { -enum DataGridFontVariant { +internal enum DataGridFontVariant {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeEngine.swift` at line 73, The types lack explicit access control; update the declarations for ThemeEngine, EditorFontCache, DataGridFontCacheResolved, and DataGridFontVariant to include an explicit access modifier (e.g., public/internal/private) consistent with their intended visibility in the module—add the modifier to the class/struct declarations (e.g., change "final class ThemeEngine" to "internal final class ThemeEngine" or "public final class ThemeEngine" as appropriate) and do the same for "EditorFontCache", "DataGridFontCacheResolved", and "DataGridFontVariant" so their visibility is explicit and consistent with usage across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/vi/development/plugin-registry.mdx`:
- Around line 103-109: The Vietnamese docs section describing theme distribution
is missing the sample theme registry JSON example; update the Vietnamese file to
include the same example JSON block found in the English version that
demonstrates a ThemeDefinition registry entry (showing category: "theme",
registry.{pluginId}.{originalSuffix} ID pattern and sample fields). Insert the
example immediately after the paragraph that explains ZIP contents and before
the sentence about Theme pack support so the ThemeDefinition example appears
where the English doc places it and mirrors the structure and fields from the
English version.
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 82-90: The code in ThemeRegistryInstaller is blocking the
`@MainActor` by calling process.waitUntilExit() after try process.run(); change
this to run the extraction off the main actor — e.g., perform the Process launch
inside Task.detached or DispatchQueue.global().async and await completion, or
use Process.terminationHandler with a checkedContinuation to resume
asynchronously; ensure you remove the synchronous waitUntilExit() and instead
await the process exit before checking process.terminationStatus so the UI
remains responsive during ditto extraction.
In `@TablePro/Theme/ThemeStorage.swift`:
- Around line 127-129: The theme loader is currently picking up
registry-meta.json as if it were a theme; modify loadThemes(from:isBuiltIn:) so
it filters out ThemeStorage.registryMetaURL (or its lastPathComponent
"registry-meta.json") when enumerating JSON files from registryThemesDirectory,
skipping that file before attempting to parse into ThemeDefinition; ensure the
same exclusion is applied in the other enumeration block referenced around lines
219–223 so registry-meta.json is never sent to ThemeDefinition decoding.
- Around line 58-67: Theme IDs are being interpolated directly into filesystem
paths (used in ThemeStorage.loadTheme, ThemeStorage.saveTheme,
ThemeStorage.deleteTheme and calls to loadTheme(from:)) enabling path traversal;
fix by validating/sanitizing IDs before constructing file paths: reject or
normalize IDs containing path separators or dot segments ("/", "..", "\\"), or
restrict IDs to a safe whitelist/regex like /^[A-Za-z0-9_-]+$/; then build paths
using userThemesDirectory.appendingPathComponent("\(safeId).json") (ensuring you
append the suffix after sanitization) and consider falling back to an encoding
strategy (percent-encode or map to a UUID) for any nonconforming IDs to
guarantee files stay inside userThemesDirectory and registryThemesDirectory.
- Line 12: Add an explicit access control modifier to the ThemeStorage type
declaration (e.g., change "struct ThemeStorage" to "internal struct
ThemeStorage" or "public struct ThemeStorage" as appropriate) and ensure any
extensions of ThemeStorage in this module also carry an explicit access modifier
on the extension declaration rather than relying on default visibility.
- Around line 182-187: The loadActiveThemeId and saveActiveThemeId functions are
using UserDefaults.standard directly (via activeThemeKey); change them to call
the app settings layer instead (AppSettingsStorage or AppSettingsManager) so
preferences go through the centralized storage API—replace
UserDefaults.standard.string(forKey: activeThemeKey) with the appropriate
read/get method on AppSettingsStorage/AppSettingsManager (e.g., the shared
instance’s string-getter for activeThemeKey) and replace
UserDefaults.standard.set(id, forKey: activeThemeKey) with the settings layer’s
write/set method for the same key, preserving the fallback default
"tablepro.default-light". Ensure you reference activeThemeKey when calling the
settings API.
In `@TablePro/Views/Settings/Appearance/ThemeEditorView.swift`:
- Around line 99-104: The duplicateAndSelect function currently swallows save
errors via try?, so change it to perform a proper do-catch around
engine.saveUserTheme(copy) (use engine.duplicateTheme and engine.activateTheme
as before) and only call engine.activateTheme(copy) and set selectedThemeId =
copy.id after a successful save; in the catch block log the error and surface a
user-facing failure (e.g., trigger the same Alert/error state used in
ThemeListView or set an `@State` errorMessage to show an alert) so the user knows
the save failed and the duplicate won’t be persisted.
In `@TablePro/Views/Settings/Appearance/ThemeListView.swift`:
- Around line 156-162: uninstallRegistryTheme currently swallows errors from
engine.uninstallRegistryTheme and always updates selectedThemeId, causing silent
failures; modify uninstallRegistryTheme (and the uninstall call site) to perform
a do-try-catch around engine.uninstallRegistryTheme(registryPluginId:), surface
the caught error to the user (e.g. present an alert or show an inline error
message) and only update selectedThemeId = engine.activeTheme.id when the
uninstall succeeds; use the existing selectedTheme,
ThemeStorage.loadRegistryMeta(), and the matched entry (meta.installed.first {
$0.id == theme.id }) to locate the registryPluginId and keep behavior otherwise
unchanged.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 10-12: There are two duplicate "### Added" headers; remove the top
one that contains the entry "Theme registry distribution: browse, install,
uninstall, and update community themes from the plugin registry (Settings >
Plugins > Browse, filtered by Themes category)" and move that bullet into the
existing "### Added" section elsewhere under [Unreleased] so all Added items are
consolidated under a single "### Added" header.
In `@TablePro/Theme/RegistryThemeMeta.swift`:
- Around line 3-16: Add explicit access control modifiers to the types and their
members: make the structs and their properties public (or internal/private as
appropriate for your module) by adding e.g. "public" to the declarations of
RegistryThemeMeta and InstalledRegistryTheme and to their stored properties and
initializer (init) so access intent is explicit; ensure Identifiable conformance
and Codable conformance remain unchanged while applying the chosen access level
to id, registryPluginId, version, installedDate and the installed property and
initializer of RegistryThemeMeta.
In `@TablePro/Theme/ThemeDefinition.swift`:
- Line 3: Add explicit access control modifiers to every type in this file
(replace implicit defaults) by prepending the appropriate access level (e.g.,
public or internal per project guidelines) to the top-level declarations such as
ThemeDefinition, ThemeAppearance, SyntaxColors, EditorThemeColors and any other
structs/enums in the file; be sure to apply the same explicit access modifier to
nested types and related typealiases so all type declarations are explicit and
consistent with the module's intended visibility.
In `@TablePro/Theme/ThemeEngine.swift`:
- Line 73: The types lack explicit access control; update the declarations for
ThemeEngine, EditorFontCache, DataGridFontCacheResolved, and DataGridFontVariant
to include an explicit access modifier (e.g., public/internal/private)
consistent with their intended visibility in the module—add the modifier to the
class/struct declarations (e.g., change "final class ThemeEngine" to "internal
final class ThemeEngine" or "public final class ThemeEngine" as appropriate) and
do the same for "EditorFontCache", "DataGridFontCacheResolved", and
"DataGridFontVariant" so their visibility is explicit and consistent with usage
across the codebase.
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Line 15: The ThemeRegistryInstaller type lacks explicit access control; update
the declaration of ThemeRegistryInstaller to include an explicit modifier (e.g.,
make ThemeRegistryInstaller either public final class ThemeRegistryInstaller or
internal final class ThemeRegistryInstaller) based on its intended visibility,
and ensure any related symbols (constructors or usages) are adjusted if you
change visibility.
- Around line 194-198: The filter currently compares versions with string
inequality (plugin.version != installed); change the comparison inside the
manifest.plugins filter closure to parse plugin.version and
installedVersions[plugin.id] into semantic-version objects (using your project's
SemVer/Version type or a lightweight parser) and compare them semantically
(e.g., Version(plugin.version) != Version(installed)). Update the filter logic
in ThemeRegistryInstaller where manifest.plugins is filtered so that unparsable
versions are handled safely (fallback to string compare or treat as unequal) and
ensure you reference plugin.version and installedVersions[plugin.id] when
creating the Version instances.
In `@TablePro/Views/Settings/Appearance/ThemeEditorView.swift`:
- Around line 34-37: Picker is currently using Text(tab.rawValue) which shows
the enum's English raw values and won't localize; update the EditorTab enum to
provide a localizedName computed property (e.g., var localizedName: String {
String(localized: ...) }) and change the Picker to use Text(tab.localizedName)
so labels are localized; ensure EditorTab conforms to CaseIterable and use the
new localizedName wherever tab display strings are needed.
- Line 10: Add an explicit access control modifier to the ThemeEditorView type
declaration (replace the implicit default on struct ThemeEditorView: View).
Update the declaration to include the intended visibility (for example "internal
struct ThemeEditorView: View" if it is module-internal, or "public struct
ThemeEditorView: View" if it is part of the module API) so the type follows the
project's explicit access-control guideline.
In `@TablePro/Views/Settings/Appearance/ThemeListRowView.swift`:
- Around line 3-4: ThemeListRowView currently has no explicit access control;
add the appropriate access modifier to the struct declaration (e.g., public or
internal) to match the module's API surface and coding guidelines. Locate the
ThemeListRowView struct and prepend the chosen access level to its declaration,
and ensure any related symbol usages (initializers or consumers of
ThemeListRowView) remain accessible under the new modifier.
In `@TablePro/Views/Settings/Appearance/ThemeListView.swift`:
- Around line 143-154: The methods duplicateActiveTheme and deleteSelectedTheme
currently swallow errors via try?; change them to use do-catch so failures from
engine.saveUserTheme and engine.deleteUserTheme are surfaced — in the catch
block log the error (via your logger) and present user-facing feedback (e.g., an
alert/toast) and avoid updating state like selectedThemeId or calling
engine.activateTheme unless the operation succeeded; ensure the code references
engine.duplicateTheme, engine.saveUserTheme, engine.activateTheme,
engine.deleteUserTheme and selectedThemeId so the UI only updates after a
successful save/delete and errors are logged and shown to the user.
- Line 5: The ThemeListView struct lacks an explicit access-control modifier;
update the declaration of ThemeListView to include an explicit modifier (e.g.,
prepend internal or the appropriate visibility such as public) so the type
conforms to project guidelines—locate the struct declaration "ThemeListView:
View" and add the chosen access level before "struct ThemeListView".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d88ca89-1490-49a2-81c2-2b544a4d6df7
📒 Files selected for processing (17)
CHANGELOG.mdTablePro/Theme/RegistryThemeMeta.swiftTablePro/Theme/ThemeDefinition.swiftTablePro/Theme/ThemeEngine.swiftTablePro/Theme/ThemeRegistryInstaller.swiftTablePro/Theme/ThemeStorage.swiftTablePro/Views/Settings/Appearance/ThemeEditorView.swiftTablePro/Views/Settings/Appearance/ThemeListRowView.swiftTablePro/Views/Settings/Appearance/ThemeListView.swiftTablePro/Views/Settings/Plugins/BrowsePluginsView.swiftTablePro/Views/Settings/Plugins/RegistryPluginDetailView.swiftdocs/customization/appearance.mdxdocs/development/plugin-registry.mdxdocs/vi/customization/appearance.mdxdocs/vi/development/plugin-registry.mdxdocs/zh/customization/appearance.mdxdocs/zh/development/plugin-registry.mdx
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
TablePro/Theme/ThemeRegistryInstaller.swift (1)
15-16: Add explicit access control on the type declaration.
ThemeRegistryInstallercurrently relies on implicitinternal. Please make the type access level explicit.As per coding guidelines, "Always use explicit access control on extensions and types (private, internal, public). Specify access modifiers on extension itself, not individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 15 - 16, ThemeRegistryInstaller currently relies on implicit internal access; update the type declaration to include an explicit access modifier by changing the declaration of ThemeRegistryInstaller (final class ThemeRegistryInstaller) to include the intended access level (e.g., internal final class ThemeRegistryInstaller or public final class ThemeRegistryInstaller as appropriate for its usage), and ensure any related extensions also have explicit access modifiers on the extension declarations rather than on individual members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 109-112: Sanitize both components when building persisted theme
IDs: add a helper like sanitizeIdentifierComponent(_:) (use a regex to replace
any char not in A-Za-z0-9_- with "-", and throw PluginError.installFailed if the
result is empty) and use it on both plugin.id and theme.id in
ThemeRegistryInstaller where you currently rewrite theme.id, and in the other
occurrence around line 120; ensure ThemeStorage.saveRegistryTheme receives only
the sanitized "registry.<sanitizedPlugin>.<sanitizedTheme>" id to prevent path
traversal or unsafe filenames.
- Around line 42-43: Replace hard-coded English error description literals in
ThemeRegistryInstaller.swift with localized strings using String(localized:).
Specifically, update each PluginError throw site (e.g.,
PluginError.downloadFailed("Invalid download URL")) and the other occurrences
around the PluginError initializers at the noted locations (lines near 59–60,
89–90, 96–97) to pass String(localized:) wrapped messages instead of plain
literals so the error descriptions are localized for user-facing UI.
- Around line 119-133: Wrap the decodedThemes installation in a transactional
block: iterate decodedThemes and call ThemeStorage.saveRegistryTheme as you do,
but collect the successfully saved theme ids (or InstalledRegistryTheme
instances) and if any saveRegistryTheme throws, catch the error, delete/cleanup
any themes already saved (call the inverse delete function for each saved theme
id), then rethrow the error; after the loop, load meta with
ThemeStorage.loadRegistryMeta, append installedThemes and call
ThemeStorage.saveRegistryMeta, and if saveRegistryMeta throws, perform the same
cleanup of all saved themes before rethrowing—use the same identifiers
(ThemeStorage.saveRegistryTheme, ThemeStorage.loadRegistryMeta,
ThemeStorage.saveRegistryMeta, InstalledRegistryTheme, decodedThemes) so the
code rolls back on any failure and avoids orphaned registry themes.
- Around line 206-209: availableUpdates is treating any version difference as an
update; change the filter to only include true upgrades by comparing versions
semantically (not just inequality). Replace the predicate return plugin.version
!= installed with a proper semantic comparison such as return plugin.version >
installed (or use your project's Version.compare/Comparable helper) so only
plugin.version newer than installedVersions[plugin.id] is returned (refer to
manifest.plugins and installedVersions[plugin.id] in the availableUpdates
filter).
- Around line 225-227: The loop that iterates the FileManager enumerator (the
for-case over enumerator producing fileURL) must skip non-regular entries before
treating .json entries as theme files; update the loop in ThemeRegistryInstaller
to query URLResourceValues (or FileManager attributes) for isRegularFile (or
ensure !hasDirectoryPath and resolve symlinks) for each fileURL and continue if
it is not a regular file, then proceed with the existing checks (pathExtension
== "json" and lastPathComponent != "registry-meta.json"); reference the
enumerator loop and the fileURL variable when making the change.
---
Nitpick comments:
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 15-16: ThemeRegistryInstaller currently relies on implicit
internal access; update the type declaration to include an explicit access
modifier by changing the declaration of ThemeRegistryInstaller (final class
ThemeRegistryInstaller) to include the intended access level (e.g., internal
final class ThemeRegistryInstaller or public final class ThemeRegistryInstaller
as appropriate for its usage), and ensure any related extensions also have
explicit access modifiers on the extension declarations rather than on
individual members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e7d5787-c44a-46a6-8c90-fef42c29fde4
📒 Files selected for processing (3)
TablePro/Theme/RegistryThemeMeta.swiftTablePro/Theme/ThemeRegistryInstaller.swiftTablePro/Views/Settings/Appearance/ThemeListView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- TablePro/Theme/RegistryThemeMeta.swift
- TablePro/Views/Settings/Appearance/ThemeListView.swift
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
TablePro/Theme/ThemeRegistryInstaller.swift (3)
41-43:⚠️ Potential issue | 🟡 MinorLocalize the new
PluginErrormessages.These error payloads are still hard-coded English literals. Wrap them in
String(localized:)before constructing the error so they can surface correctly in localized UI.Based on learnings, "Use String(localized:) for new user-facing strings in computed properties, AppKit code, alerts, and error descriptions. SwiftUI view literals (Text("literal"), Button("literal")) auto-localize. Do not localize technical terms (font names, database types, SQL keywords, encoding names)."
Also applies to: 56-60, 88-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 41 - 43, The PluginError messages are hard-coded English literals; update all constructions of PluginError (e.g., PluginError.downloadFailed("Invalid download URL") and similar instances around the blocks that construct errors at the URL(string: resolved.url) guard and the other spots noted) to wrap the user-facing message in String(localized:), e.g. use String(localized: "Invalid download URL"), so replace the literal strings passed into PluginError initializers with String(localized:) calls; ensure only user-facing text is localized and technical identifiers (like resolved.url or font names) remain unchanged.
225-229:⚠️ Potential issue | 🟡 MinorIgnore non-regular
.jsonentries from the extracted ZIP.The enumerator still appends any path ending in
.json. A directory or symlink with that suffix will be treated as a theme file and can break the install path.Suggested guard
for case let fileURL as URL in enumerator { - if fileURL.pathExtension.lowercased() == "json" && + let values = try fileURL.resourceValues(forKeys: [.isRegularFileKey]) + guard values.isRegularFile == true else { continue } + + if fileURL.pathExtension.lowercased() == "json" && fileURL.lastPathComponent != "registry-meta.json" { results.append(fileURL) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 225 - 229, The enumerator currently appends any URL ending with ".json" including directories or symlinks; update the loop in ThemeRegistryInstaller (where you call fileURL.resourceValues and results.append) to skip non-regular files by fetching resource values for .isRegularFileKey and .isSymbolicLinkKey (e.g. let vals = try? fileURL.resourceValues(forKeys: [.isRegularFileKey, .isSymbolicLinkKey])) and add a guard like guard vals?.isRegularFile == true && vals?.isSymbolicLink != true else { continue } before calling results.append(fileURL).
119-133:⚠️ Potential issue | 🟠 MajorRollback written theme files if the install cannot finish.
ThemeStorage.saveRegistryThemepersists JSON immediately, whileThemeStorage.loadAllThemes()later loads registry themes straight from disk. If any later write fails, the app can surface orphaned registry themes with no matching installed-version metadata.Suggested rollback pattern
- var installedThemes: [InstalledRegistryTheme] = [] - - for theme in decodedThemes { - try ThemeStorage.saveRegistryTheme(theme) - - installedThemes.append(InstalledRegistryTheme( - id: theme.id, - registryPluginId: plugin.id, - version: plugin.version, - installedDate: Date() - )) - } - - // Update meta - var meta = ThemeStorage.loadRegistryMeta() - meta.installed.append(contentsOf: installedThemes) - try ThemeStorage.saveRegistryMeta(meta) + var installedThemes: [InstalledRegistryTheme] = [] + var writtenThemeIds: [String] = [] + + do { + for theme in decodedThemes { + try ThemeStorage.saveRegistryTheme(theme) + writtenThemeIds.append(theme.id) + + installedThemes.append(InstalledRegistryTheme( + id: theme.id, + registryPluginId: plugin.id, + version: plugin.version, + installedDate: Date() + )) + } + + var meta = ThemeStorage.loadRegistryMeta() + meta.installed.append(contentsOf: installedThemes) + try ThemeStorage.saveRegistryMeta(meta) + } catch { + for id in writtenThemeIds { + try? ThemeStorage.deleteRegistryTheme(id: id) + } + throw error + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 119 - 133, The current loop calls ThemeStorage.saveRegistryTheme for each theme and then saves meta with ThemeStorage.saveRegistryMeta, which can leave theme JSON files orphaned if a later write fails; update the install flow (the loop handling decodedThemes, InstalledRegistryTheme creation, and subsequent meta update) to perform a transactional install: track which theme IDs were successfully persisted, attempt all saves (including ThemeStorage.saveRegistryMeta) inside a do/catch, and if any save throws, roll back by deleting the persisted registry theme files for those tracked IDs (use ThemeStorage.deleteRegistryTheme or the equivalent removal code) before rethrowing or returning an error; ensure InstalledRegistryTheme list is only appended/committed after all saves succeed so loadAllThemes() and saved meta remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Theme/ThemeDefinition.swift`:
- Around line 588-598: ThemeEdgeInsets currently uses synthesized Codable which
fails when only some edges are provided; implement a custom init(from decoder:)
that decodes a keyed container and uses decodeIfPresent for top, leading,
bottom, and trailing, falling back to ThemeEdgeInsets.default.<properties>
(refer to ThemeEdgeInsets, its static `default`, and properties
top/leading/bottom/trailing) to populate missing values, and add a matching
encode(to:) using the same CodingKeys if you need explicit encoding symmetry.
In `@TablePro/Theme/ThemeEngine.swift`:
- Around line 174-179: The duplicateTheme(_ theme: ThemeDefinition, newName:
String) function should not inject NSFullUserName() into the duplicated
ThemeDefinition because ThemeStorage.exportTheme will serialize that personal
data; remove the assignment copy.author = NSFullUserName() and preserve the
existing theme.author (i.e., leave copy.author unchanged), or only set a
non-identifying default if theme.author is empty, so the author field is not
populated with the macOS full user name upon duplication.
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 173-179: The update(_ plugin: RegistryPlugin, ...) implementation
is destructive because it calls uninstall(registryPluginId:) before
install(...); change the flow to stage the new plugin first and only swap it in
once install completes successfully: have update call install(plugin, progress:)
into a temporary/staging location (or use a new installAsyncStaging method),
verify checksum/extraction/decoding, then atomically replace the old package and
update ThemeEngine.shared.activeTheme as needed, and only call
uninstall(registryPluginId:) (or remove the old files) after the new version is
fully staged; ensure failures during install leave the existing active theme
intact and perform cleanup of the staging area on error to avoid leaving partial
state.
- Around line 109-112: The current ID rewrite in ThemeRegistryInstaller
(replacing "." with "-") is lossy and can collapse distinct IDs (e.g.,
"solarized.dark" vs "solarized-dark"); instead preserve a reversible mapping by
keeping the original theme.id suffix (do not replace "."), e.g. set theme.id =
"registry.\(plugin.id).\(theme.id)" or apply a reversible encoder
(percent-encode) if dots must be escaped, and before persisting use
ThemeStorage.themeFileURL to detect any existing file collision and
reject/handle it (throw or rename) to avoid overwriting another theme; update
the code that currently computes sanitizedId to use the non-destructive approach
and add a collision check against ThemeStorage.themeFileURL prior to write.
In `@TablePro/Theme/ThemeStorage.swift`:
- Around line 161-170: importTheme currently preserves non-built-in theme.id and
calls saveUserTheme, which silently overwrites an existing custom theme with the
same id; to fix, before calling saveUserTheme in importTheme, check whether a
user theme with theme.id already exists (e.g., using your storage
lookup/file-exists helper or the same logic saveUserTheme uses to compute the
destination filename), and if it does either (a) generate a new unique id (e.g.,
"user.\(UUID...)" or append a short suffix) and assign it to theme.id, or (b)
surface an error to the caller so they can choose to replace; ensure the
check/update happens inside importTheme before calling saveUserTheme so no
existing user theme is overwritten.
---
Duplicate comments:
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 41-43: The PluginError messages are hard-coded English literals;
update all constructions of PluginError (e.g.,
PluginError.downloadFailed("Invalid download URL") and similar instances around
the blocks that construct errors at the URL(string: resolved.url) guard and the
other spots noted) to wrap the user-facing message in String(localized:), e.g.
use String(localized: "Invalid download URL"), so replace the literal strings
passed into PluginError initializers with String(localized:) calls; ensure only
user-facing text is localized and technical identifiers (like resolved.url or
font names) remain unchanged.
- Around line 225-229: The enumerator currently appends any URL ending with
".json" including directories or symlinks; update the loop in
ThemeRegistryInstaller (where you call fileURL.resourceValues and
results.append) to skip non-regular files by fetching resource values for
.isRegularFileKey and .isSymbolicLinkKey (e.g. let vals = try?
fileURL.resourceValues(forKeys: [.isRegularFileKey, .isSymbolicLinkKey])) and
add a guard like guard vals?.isRegularFile == true && vals?.isSymbolicLink !=
true else { continue } before calling results.append(fileURL).
- Around line 119-133: The current loop calls ThemeStorage.saveRegistryTheme for
each theme and then saves meta with ThemeStorage.saveRegistryMeta, which can
leave theme JSON files orphaned if a later write fails; update the install flow
(the loop handling decodedThemes, InstalledRegistryTheme creation, and
subsequent meta update) to perform a transactional install: track which theme
IDs were successfully persisted, attempt all saves (including
ThemeStorage.saveRegistryMeta) inside a do/catch, and if any save throws, roll
back by deleting the persisted registry theme files for those tracked IDs (use
ThemeStorage.deleteRegistryTheme or the equivalent removal code) before
rethrowing or returning an error; ensure InstalledRegistryTheme list is only
appended/committed after all saves succeed so loadAllThemes() and saved meta
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d609fea5-53c7-4e32-8db3-67a66d7e516c
📒 Files selected for processing (10)
CHANGELOG.mdTablePro/Theme/ThemeDefinition.swiftTablePro/Theme/ThemeEngine.swiftTablePro/Theme/ThemeRegistryInstaller.swiftTablePro/Theme/ThemeStorage.swiftTablePro/Views/Settings/Appearance/ThemeEditorView.swiftTablePro/Views/Settings/Appearance/ThemeListRowView.swiftTablePro/Views/Settings/Appearance/ThemeListView.swiftdocs/vi/development/plugin-registry.mdxdocs/zh/development/plugin-registry.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- TablePro/Views/Settings/Appearance/ThemeListView.swift
- TablePro/Views/Settings/Appearance/ThemeEditorView.swift
There was a problem hiding this comment.
🧹 Nitpick comments (4)
TablePro/Theme/ThemeRegistryInstaller.swift (2)
255-260: Filter to regular files before treating as theme JSON.The enumerator requests
isRegularFileKeybut doesn't check the resource value. A directory or symlink ending in.jsonwould be included and cause decode failures.Suggested fix
for case let fileURL as URL in enumerator { + let values = try? fileURL.resourceValues(forKeys: [.isRegularFileKey]) + guard values?.isRegularFile == true else { continue } if fileURL.pathExtension.lowercased() == "json" && fileURL.lastPathComponent != "registry-meta.json" { results.append(fileURL) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 255 - 260, The enumerator loop that collects JSON theme files (the `for case let fileURL as URL in enumerator` block) currently only filters by path extension and name; update it to query the file resource values for the requested key (`.isRegularFileKey`) and skip any URL where `isRegularFile` is false (or the resource lookup fails), so directories or symlinks ending in `.json` are not appended to `results` (i.e., call `fileURL.resourceValues(forKeys: [.isRegularFileKey])` and only append when the retrieved `isRegularFile == true`).
36-55: Consider making installation transactional to handle partial failures.If
saveRegistryThemefails mid-loop orsaveRegistryMetathrows, already-written theme files remain while metadata may be inconsistent. A rollback mechanism would improve reliability:Suggested rollback approach
var installedThemes: [InstalledRegistryTheme] = [] + var writtenThemeIds: [String] = [] - for theme in decodedThemes { - try ThemeStorage.saveRegistryTheme(theme) - - installedThemes.append(InstalledRegistryTheme( - id: theme.id, - registryPluginId: plugin.id, - version: plugin.version, - installedDate: Date() - )) + do { + for theme in decodedThemes { + try ThemeStorage.saveRegistryTheme(theme) + writtenThemeIds.append(theme.id) + + installedThemes.append(InstalledRegistryTheme( + id: theme.id, + registryPluginId: plugin.id, + version: plugin.version, + installedDate: Date() + )) + } + + var meta = ThemeStorage.loadRegistryMeta() + meta.installed.append(contentsOf: installedThemes) + try ThemeStorage.saveRegistryMeta(meta) + } catch { + for id in writtenThemeIds { + try? ThemeStorage.deleteRegistryTheme(id: id) + } + throw error } - - var meta = ThemeStorage.loadRegistryMeta() - meta.installed.append(contentsOf: installedThemes) - try ThemeStorage.saveRegistryMeta(meta)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeRegistryInstaller.swift` around lines 36 - 55, The loop writing themes is not atomic: if ThemeStorage.saveRegistryTheme or ThemeStorage.saveRegistryMeta fails you can end up with files but inconsistent meta; change install logic in the function that iterates decodedThemes so that you first write each theme to a temporary/staged location (use ThemeStorage.saveRegistryTheme to a temp path or use a staging flag), collect InstalledRegistryTheme entries, then only when all saves succeed update meta via ThemeStorage.loadRegistryMeta and ThemeStorage.saveRegistryMeta and move/stage the temp themes into their final location; wrap the whole operation in a do/catch and on any error delete any staged/partially-saved themes and do not persist meta, then rethrow or return a failure, and only call ThemeEngine.shared.reloadAvailableThemes() and progress(1.0) after a successful commit.TablePro/Theme/ThemeEngine.swift (2)
320-332: Add explicit access control to extension.Per coding guidelines, extensions should have explicit access modifiers. Consider
internal extension View.Suggested fix
-extension View { +internal extension View { func cardStyle() -> some View { self .background(ThemeEngine.shared.colors.ui.controlBackgroundSwiftUI) .clipShape(RoundedRectangle(cornerRadius: ThemeEngine.shared.activeTheme.cornerRadius.medium)) } func toolbarButtonStyle() -> some View { self .buttonStyle(.borderless) .foregroundStyle(.secondary) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeEngine.swift` around lines 320 - 332, The extension on View lacks an explicit access level; change it to an explicit internal extension (e.g., "internal extension View") so the access control is clear for the extension and its methods (cardStyle() and toolbarButtonStyle()); ensure the modifier is applied to the extension declaration (not individual methods) so ThemeEngine.shared usage and the existing implementations remain unchanged.
312-316: Add explicit access control to extension.Per coding guidelines, extensions should have explicit access modifiers. Consider
internal extension DatabaseType.Suggested fix
-extension DatabaseType { +internal extension DatabaseType { `@MainActor` var themeColor: Color { PluginManager.shared.brandColor(for: self) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeEngine.swift` around lines 312 - 316, The extension on DatabaseType lacks an explicit access modifier; update it to use an explicit access level (e.g., change to "internal extension DatabaseType") while keeping the `@MainActor` var themeColor and its body (PluginManager.shared.brandColor(for: self)) unchanged so the access control is explicit without altering behavior. Ensure the access level chosen follows project guidelines and that the declaration precedes the `@MainActor` var themeColor within the extension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Theme/ThemeEngine.swift`:
- Around line 320-332: The extension on View lacks an explicit access level;
change it to an explicit internal extension (e.g., "internal extension View") so
the access control is clear for the extension and its methods (cardStyle() and
toolbarButtonStyle()); ensure the modifier is applied to the extension
declaration (not individual methods) so ThemeEngine.shared usage and the
existing implementations remain unchanged.
- Around line 312-316: The extension on DatabaseType lacks an explicit access
modifier; update it to use an explicit access level (e.g., change to "internal
extension DatabaseType") while keeping the `@MainActor` var themeColor and its
body (PluginManager.shared.brandColor(for: self)) unchanged so the access
control is explicit without altering behavior. Ensure the access level chosen
follows project guidelines and that the declaration precedes the `@MainActor` var
themeColor within the extension.
In `@TablePro/Theme/ThemeRegistryInstaller.swift`:
- Around line 255-260: The enumerator loop that collects JSON theme files (the
`for case let fileURL as URL in enumerator` block) currently only filters by
path extension and name; update it to query the file resource values for the
requested key (`.isRegularFileKey`) and skip any URL where `isRegularFile` is
false (or the resource lookup fails), so directories or symlinks ending in
`.json` are not appended to `results` (i.e., call
`fileURL.resourceValues(forKeys: [.isRegularFileKey])` and only append when the
retrieved `isRegularFile == true`).
- Around line 36-55: The loop writing themes is not atomic: if
ThemeStorage.saveRegistryTheme or ThemeStorage.saveRegistryMeta fails you can
end up with files but inconsistent meta; change install logic in the function
that iterates decodedThemes so that you first write each theme to a
temporary/staged location (use ThemeStorage.saveRegistryTheme to a temp path or
use a staging flag), collect InstalledRegistryTheme entries, then only when all
saves succeed update meta via ThemeStorage.loadRegistryMeta and
ThemeStorage.saveRegistryMeta and move/stage the temp themes into their final
location; wrap the whole operation in a do/catch and on any error delete any
staged/partially-saved themes and do not persist meta, then rethrow or return a
failure, and only call ThemeEngine.shared.reloadAvailableThemes() and
progress(1.0) after a successful commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf339c46-3964-4648-baf7-d3c7c3390c26
📒 Files selected for processing (4)
TablePro/Theme/ThemeDefinition.swiftTablePro/Theme/ThemeEngine.swiftTablePro/Theme/ThemeRegistryInstaller.swiftTablePro/Theme/ThemeStorage.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
TablePro/Theme/ThemeStorage.swift (1)
186-191:⚠️ Potential issue | 🟠 MajorPersist the active theme through the app settings layer.
These reads and writes still bypass
AppSettingsStorage/AppSettingsManager, so the active-theme preference is handled differently from the rest of the app settings.As per coding guidelines: "Store user preferences in UserDefaults via AppSettingsStorage/AppSettingsManager."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeStorage.swift` around lines 186 - 191, The loadActiveThemeId and saveActiveThemeId functions are directly using UserDefaults and must instead delegate to the app settings layer; replace the UserDefaults calls in loadActiveThemeId() and saveActiveThemeId(_:) to read/write the same key (activeThemeKey) via the AppSettingsStorage/AppSettingsManager API used elsewhere (e.g., AppSettingsStorage.shared / AppSettingsManager.shared or the project's equivalent singletons), returning the default "tablepro.default-light" when the stored value is nil and calling the settings manager's set/update method to persist the id so active-theme preference uses the same storage abstraction as other app settings.
🧹 Nitpick comments (1)
TablePro/Theme/ThemeEngine.swift (1)
312-331: Declare explicit access control on these extensions.Both extensions rely on implicit
internal. Please put the access modifier on the extension declarations themselves so the surface stays explicit.As per coding guidelines: "Always use explicit access control on extensions and types (private, internal, public). Specify access modifiers on extension itself, not individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeEngine.swift` around lines 312 - 331, Mark the two extensions with explicit access control instead of relying on implicit internal: add an access modifier (likely internal or public per module policy) to the extension on DatabaseType (the extension that defines the `@MainActor` var themeColor and calls PluginManager.shared.brandColor(for:)) and to the extension on View (the extension that defines cardStyle() and toolbarButtonStyle()). Update the extension declarations themselves (not the individual members) so the surface is explicit and consistent with the project's access-control guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Theme/ThemeEngine.swift`:
- Around line 111-113: The code currently picks an active Theme by matching
activeId against the array from ThemeStorage.loadAllThemes(), which can pick the
wrong copy when an ID exists in multiple sources; instead call
ThemeStorage.loadTheme(id: activeId) to resolve the ID using the correct
precedence (user → registry → bundle). Update the logic that computes the active
theme (the block using ThemeStorage.loadAllThemes() and
ThemeStorage.loadActiveThemeId()) to call ThemeStorage.loadTheme(id:) and fall
back to .default if it returns nil; make the same change in the similar block
around ThemeStorage.loadAllThemes()/loadActiveThemeId() at the later occurrence
(lines mentioned in the review) so both active-theme lookups use
ThemeStorage.loadTheme(id:).
---
Duplicate comments:
In `@TablePro/Theme/ThemeStorage.swift`:
- Around line 186-191: The loadActiveThemeId and saveActiveThemeId functions are
directly using UserDefaults and must instead delegate to the app settings layer;
replace the UserDefaults calls in loadActiveThemeId() and saveActiveThemeId(_:)
to read/write the same key (activeThemeKey) via the
AppSettingsStorage/AppSettingsManager API used elsewhere (e.g.,
AppSettingsStorage.shared / AppSettingsManager.shared or the project's
equivalent singletons), returning the default "tablepro.default-light" when the
stored value is nil and calling the settings manager's set/update method to
persist the id so active-theme preference uses the same storage abstraction as
other app settings.
---
Nitpick comments:
In `@TablePro/Theme/ThemeEngine.swift`:
- Around line 312-331: Mark the two extensions with explicit access control
instead of relying on implicit internal: add an access modifier (likely internal
or public per module policy) to the extension on DatabaseType (the extension
that defines the `@MainActor` var themeColor and calls
PluginManager.shared.brandColor(for:)) and to the extension on View (the
extension that defines cardStyle() and toolbarButtonStyle()). Update the
extension declarations themselves (not the individual members) so the surface is
explicit and consistent with the project's access-control guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 924cdde1-6b57-4a20-893a-b236ea141f04
📒 Files selected for processing (2)
TablePro/Theme/ThemeEngine.swiftTablePro/Theme/ThemeStorage.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.tablepluginbundles, no code signing neededtablepro.*), Registry (registry.*), User (user.*) with clean ID namespacingChanges
New files
RegistryThemeMeta.swift— version tracking for installed registry themesThemeRegistryInstaller.swift—@MainActor @Observablesingleton: download ZIP, verify SHA-256, extract JSONs, rewrite IDs, manage install/uninstall/update lifecycleModified files
ThemeDefinition.swift— addedisRegistryandisEditablecomputed propertiesThemeStorage.swift— registry themes directory, CRUD methods, meta persistenceThemeEngine.swift—registryThemescomputed property,uninstallRegistryTheme(), blocked delete for registry IDsThemeListView.swift— "Registry" section, uninstall option in gear menuThemeListRowView.swift— "Registry" subtitle labelThemeEditorView.swift— usestheme.isEditable, context-aware duplicate promptBrowsePluginsView.swift— routes theme installs toThemeRegistryInstallerRegistryPluginDetailView.swift— "Install Theme" button text, installed state for themesDocs
Test plan
xcodebuild -skipPackagePluginValidationSummary by CodeRabbit
New Features
Documentation