Repository navigation
[v3] Built-in service standardisation and enhancement - #4067
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the documentation and service implementations across the codebase. It revises the changelog to document new features, breaking changes, and method renames. Several services—including fileserver, kvstore, sqlite, and log—have been refactored to adopt a consistent configuration approach using methods like Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant FS as Fileserver Service
Client->>FS: Call Configure(config)
FS->>FS: Set atomic HTTP handler (or dummyHandler if config is nil)
Client->>FS: Send HTTP Request
FS->>Client: Serve request using configured handler (or return 503)
sequenceDiagram
participant App as Application
participant SQLite as SQLite Service
App->>SQLite: Call PrepareContext("SQL Query")
SQLite->>SQLite: Create prepared statement (Stmt)
App->>SQLite: Call ExecContext/QueryContext with prepared Stmt and args
SQLite->>App: Return execution result or query result
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
docs/src/content/docs/changelog.mdx (1)
40-40:⚠️ Potential issueRemove duplicate entry.
The "New Menu guide" entry appears twice.
Apply this diff to remove the duplicate:
- New Menu guide by [@leaanthony](https://github.com/leaanthony) - Better panic handling by [@leaanthony](https://github.com/leaanthony) - - New Menu guide by [@leaanthony](https://github.com/leaanthony)Also applies to: 42-42
🧹 Nitpick comments (18)
v3/pkg/services/fileserver/fileserver.go (3)
22-26: Consider validating config in the constructor
While it’s fine to delegate all logic toConfigure, adding basic validations (e.g., checking for invalidRootPath) can help catch errors early.
40-45: Handler reconfiguration design
Storing thehttp.FileServerordummyHandlervia an atomic pointer is concurrency-safe. This approach is consistent with the new design, but consider logging or returning an error ifRootPathis invalid.
52-54: Clear fallback response
Returning 503 is a clear indication that the service is not configured, but consider providing more descriptive instructions to the client on how to configure it.v3/pkg/services/log/log.go (2)
49-54: Helper constructor
Consider validating or cloning the config as soon as it's passed in, to ensure external modifications do not affect theService.
67-83: Thread-safe configuration
Cloning the config to avoid external mutations is a good practice. However, consider logging if invalid levels or other anomalies are detected.v3/pkg/services/kvstore/kvstore.go (4)
17-20: AutoSave mechanism
The auto-save explanation is helpful. Ensure that frequent writes are carefully managed for performance.
38-47: Initialize store with configuration
Creating an empty map upfront avoids nil checks later. Good approach.
You might want to log or handle missing file paths ifconfigis provided but no filename is set.
93-125: File loading logic
Handling a missing file gracefully is user-friendly, but consider logging that the file doesn’t exist so users know why the data is empty.
128-136: On-disk persistence
Using JSON is a straightforward approach. Ensure large datasets do not cause performance issues or memory overhead.v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/service.js (2)
9-12: Consider removing @ts-ignore
You have an import of$modelsthat is used below, so you may not need the@ts-ignoredirective. If$modelsis definitely used, you can safely remove theUnused importsdirective.
35-38: Add null guard
To fully support the doc statement about idempotency, you could add a null guard to handlenullor already closed statements gracefully.v3/pkg/services/sqlite/sqlite.go (4)
59-59: Added concurrency fields
Thelock sync.RWMutexandstmts map[uint64]struct{}ensure safe concurrent usage. Keep an eye on read locks to prevent potential writer starvation.Also applies to: 62-62
125-159:Openmethod
Ensures aDBSourceis set, closes any existing connection, and opens a new one. The flow is correct. Consider logging any errors for better observability.
276-326:PrepareContext
Generates a new statement ID usingnextId. The approach is robust, but watch out for potential ID exhaustion in extremely long-lived services.
428-437: Global concurrency structures
stmts sync.MapandnextIdare shared globally. Revisit the design if memory usage grows over time or if multiple service instances are used concurrently.v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/service.js (1)
66-69: Consider renaming theSetfunction to avoid shadowing.The
Setfunction shadows the globalSetobject, which could lead to confusion. Consider using a more specific name likeSetValueorSetKey.-export function Set(key, value) { +export function SetValue(key, value) { let $resultPromise = /** @type {any} */($Call.ByID(2491766752, key, value)); return $resultPromise; }🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Do not shadow the global "Set" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
docs/src/content/docs/learn/services.mdx (1)
471-471: Fix grammatical issues in the documentation.Replace "an hardcoded" with "a hardcoded" in the following lines:
- Line 471: "we used an hardcoded route"
- Line 580: "in place of an hardcoded path"
Also applies to: 580-580
docs/src/content/docs/tutorials/01-creating-a-service.mdx (1)
212-212: Fix grammatical issue in the tutorial.Add the missing pronoun "you" in the sentence:
- Line 212: "If have a vanilla frontend" should be "If you have a vanilla frontend"
🧰 Tools
🪛 LanguageTool
[grammar] ~212-~212: It seems that a pronoun is missing.
Context: ...lable when using frontend bundlers. If have a vanilla frontend that does not e...(IF_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
docs/src/content/docs/changelog.mdx(2 hunks)docs/src/content/docs/learn/services.mdx(1 hunks)docs/src/content/docs/tutorials/01-creating-a-service.mdx(12 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/index.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/keyvaluestore.js(0 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/service.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/index.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/loggerservice.js(0 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/models.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/service.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/index.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/models.js(1 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/service.js(2 hunks)v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/stmt.js(1 hunks)v3/examples/services/assets/bindings/log/slog/index.js(0 hunks)v3/examples/services/assets/index.html(3 hunks)v3/examples/services/main.go(2 hunks)v3/pkg/application/logger_dev.go(1 hunks)v3/pkg/application/logger_dev_windows.go(1 hunks)v3/pkg/application/logger_prod.go(1 hunks)v3/pkg/services/fileserver/fileserver.go(2 hunks)v3/pkg/services/kvstore/kvstore.go(2 hunks)v3/pkg/services/log/log.go(1 hunks)v3/pkg/services/sqlite/sqlite.go(3 hunks)v3/pkg/services/sqlite/stmt.js(1 hunks)
💤 Files with no reviewable changes (3)
- v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/keyvaluestore.js
- v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/loggerservice.js
- v3/examples/services/assets/bindings/log/slog/index.js
✅ Files skipped from review due to trivial changes (2)
- v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/index.js
- v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/models.js
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/tutorials/01-creating-a-service.mdx
[grammar] ~212-~212: It seems that a pronoun is missing.
Context: ...lable when using frontend bundlers. If have a vanilla frontend that does not e...
(IF_VB)
[misspelling] ~471-~471: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...tions In the example above we used an hardcoded route /qrservice. If yo...
(EN_A_VS_AN)
[misspelling] ~580-~580: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...jsto use theURL` method in place of an hardcoded path: ```js title="front...
(EN_A_VS_AN)
🪛 Biome (1.9.4)
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/service.js
[error] 66-66: Do not shadow the global "Set" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.23)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (96)
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/index.js (2)
5-8: LGTM! Service standardization looks good.The changes align with the PR objectives of standardizing built-in services by introducing a consistent
Servicetype.
10-12: LGTM! Level type export looks good.The addition of the
Leveltype export aligns with the PR objectives of enhancing the logging service with a new logging level type.v3/pkg/services/fileserver/fileserver.go (6)
5-5: No immediate concerns about concurrency usage
The import ofsync/atomicis appropriate for safe concurrent updates of the handler pointer.
9-9: Doc comment looks good
The added doc line clarifies howRootPathis used.
14-14: Ensure pointer defaults are always set
Storing a dummy handler is a good fallback and helps avoid nil pointer dereferences.
17-19: Chaining constructor approach
ReturningNewWithConfig(nil)fromNew()is a neat approach for DRY.
35-39: Comprehensive documentation
The doc comments here clearly explain the fallback behavior if config is nil.
49-49: Safe atomic load usage
Using(*s.fs.Load()).ServeHTTP(w, r)ensures concurrency correctness. Good job.v3/pkg/services/log/log.go (11)
7-7: Synchronized configuration pointer
Importing"sync/atomic"is consistent with the approach of usingatomic.Pointer[Config]for concurrency.
12-23: Clear definition of log levels
MappingLeveltoslog.Levelensures easy integration with built-in severity levels. Nice approach.
33-41: Wails injection approach
Exporting the context-bound methods as top-level is a neat design for bridging.
44-47: New service function
Chaining toNewWithConfig(nil)keeps constructor logic centralized.
56-60: Simple service naming
Returning a consistent plugin name is straightforward and aligns with the typical pattern. No concerns.
62-66: Comprehensive reconfiguration doc
The doc comments precisely explain the fallback when config is nil. Good clarity.
85-111: Extensive documentation
The doc block is very thorough. It’s helpful in explaining the dynamic log level usage.
113-118: Convenient alias
ExposingLogLevel()as an alias for the dynamic setting is user-friendly.
120-123: Runtime log level updates
Being able to adjust the log level at runtime is a powerful feature.
125-139: Flexible logging
Feeding the call context into the logger may be beneficial for structured logging. No issues found.
141-195: Convenience logging methods
Providing typed methods for each log level is a best practice and improves code readability.v3/pkg/services/kvstore/kvstore.go (11)
14-14: Documentation for on-disk store
Clarity on the filename usage is good. This helps users understand how the store is persisted.
24-31: Service struct refactor
Lock-based concurrency combined with an unsaved flag is a straightforward design.
Consider anatomic.Boolforunsavedif you want more granular updates.
33-36: Default in-memory store
By returningNewWithConfig(nil), you ensure consistent initialization.
51-51: Explicit plugin name
No concerns with the updated service name.
55-59: Service startup loading
Wrapping the error fromLoad()ensures clarity on any failing condition. Good error-handling approach.
61-65: Service shutdown saving
Similarly wrapping theSave()error clarifies if there’s an issue persisting data at shutdown.
67-91: Dynamic reconfiguration
Preserving existing store data while reconfiguring is sensible. Note that if the store was unsaved, changes might be lost unlessSave()was called.Do you intend to automatically save before reconfiguring, or is the manual approach enough?
152-164: Retrieve operation
Returning the entire store whenkeyis empty is a convenient feature. Just ensure there’s no privacy or security risk.
165-184: Set with autosave
Encapsulating everything in a closure ensures consistent locking. Well done.
186-206: Delete with autosave
Same pattern as Set. Straightforward and consistent.
208-229: Clear with autosave
Resettingdatato a new map is simpler than manually deleting each key. Looks good.v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/service.js (15)
14-20: Documentation clarity
The doc block forCloseis comprehensive. Consider explicitly mentioning concurrency behavior ifCloseis called multiple times concurrently.
28-34: Validate 'stmt' argument
The doc statesClosePreparedis idempotent, but there's no explicit check fornullin the function. Consider verifying ifstmtis non-null to avoid potential errors during$Call.ByID.
40-46: Doc coverage
The documentation forExecContextis consistent and clear on early-cancellation behavior.
47-50: Implementation clarity
The usage of spread syntax forargsis straightforward, and there is no apparent concurrency issue.
52-60: Doc completeness
ExecPrepareddoc clearly outlines usage and cancellation. Good job.
61-64: Implementation matches doc
The function logically aligns with the published doc.
66-79: Concurrent calls
This doc forOpen()is thorough. However, ensure to handle repeated calls from multiple threads, to preserve data integrity.
81-92: Doc presentation
PrepareContextdoc has sufficient detail on concurrency, closure, and usage patterns.
93-96: Function usage
PrepareContexthere is standard. No concerns.
98-108: QueryContext doc
Explicitly stating partial result returns on cancellation is valuable.
109-113: Typed results
Returning$models.Rowsis consistent with other methods. No issues.
115-131: Thorough explanation
QueryPrepareddoc is well-structured and clarifies concurrency.
137-140: Naming consistency
Re-exportingExecContextasExecuteandQueryContextasQuerymight confuse new devs but presumably maintains backward compatibility. Consider adding a comment clarifying.
142-143: ImportedStmt
ImportingStmtfrom./stmt.jsis consistent with your approach to prepared statements.
144-167: Wrapper design
Preparefunction elegantly wrapsPrepareContext. Overall design is cohesive.v3/pkg/services/sqlite/sqlite.go (24)
5-5: Import usage
The new imports (bytes, fmt, sync, atomic, errors) are used appropriately for concurrency control and error wrapping.Also applies to: 8-10, 12-12
18-22: DBSource doc
Clear explanation of how to use:memory:and parameterization withmodernc.org/sqlite.
25-48: Generated code
These wails:inject lines appear to be auto-generated bindings.
65-68:Newfunction
Initializing the service with default config is a user-friendly approach.
70-81:NewWithConfigfunction
Falling back to an in-memory database is a sensible default.
89-93: ServiceStartup
Automatically callingOpen()is consistent with the design. Consider robust error handling ifOpen()fails.
95-99: ServiceShutdown
CallsClose()for cleanup. Make sure the application handles any returned errors gracefully.
101-123:Configuremethod
Preserves open connections while changing config. The doc clarifies the reconnection step. Implementation is neat.
161-167:Closemethod
LeveragescloseImpl()under a lock. The concurrency approach is sound.
174-200:closeImpl
Iterates overs.stmtsand closes them. The approach to skip callingStmt.Close()directly to avoid deadlocks is wise.
202-208:Execute
Simple wrapper forExecContext. Implementation is consistent with typical designs.
209-229:ExecContext
Cancels gracefully ifctxis canceled. Good use of read lock for concurrency.
230-236:Query
Wrapper forQueryContext. Straightforward approach.
238-262:QueryContext
Partial result return on context cancellation is user-friendly. Ensures no open connection error is returned.
264-274:Prepare
UsesPrepareContextbehind the scenes. Clean code.
328-336:ClosePrepared
Delegates tostmt.Close(). Straight to the point.
338-357:ExecPrepared
Checks for null statements, gracefully handles context cancellation. Good design.
359-382:QueryPrepared
Follows the same pattern asExecPrepared. The final partial result approach is consistent.
384-391:RowandRows
Usingmap[string]anyfor each row is a flexible approach.
394-426:parseRows
Implementation scans each row into a map. The context check insideNext()loop is a nice touch for partial results.
438-440:init()
InitializesnextIdto 1. Straightforward.
442-454:Stmtstruct
Associates anidfor concurrency tracking. The design is well-structured for prepared statements.
456-474:Stmt.Close
Properly removes the statement from global sync.Map and local map. Watch for potential repeated calls if a context is canceled mid-statement.
476-500:MarshalText,UnmarshalText
Enables statements to be transferred or stored by ID. This is helpful for advanced usage. Implementation seems correct.v3/pkg/application/logger_prod.go (1)
10-10: Flexibility in logging level
Switching fromslog.Leveltoslog.Levelerbroadens the range of accepted log levels. Good improvement.v3/pkg/application/logger_dev.go (1)
14-14: Great improvement in flexibility!The change from
slog.Leveltoslog.Levelerinterface type makes the function more flexible while maintaining backward compatibility, as it allows for both built-in and custom log levels.v3/pkg/application/logger_dev_windows.go (1)
15-15: LGTM!The changes are consistent with
logger_dev.go, making the function more flexible by accepting theslog.Levelerinterface.v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/index.js (1)
10-22:Details
✅ Verification successful
LGTM! Verify auto-generation.
The added type definitions enhance type safety for SQLite query results. Since this is an auto-generated file, ensure that the generation process is working correctly.
Run the following script to verify the auto-generation process:
🏁 Script executed:
#!/bin/bash # Description: Verify that the file is auto-generated correctly. # Test: Check if there are any other auto-generated files with similar patterns fd -e js -x grep -l "This file is automatically generated" {}Length of output: 66233
Auto-generation process confirmed – no changes required.
The file under review has been verified against the auto-generation signature across the codebase. The executed script found this file among other auto-generated files, confirming that the generation process is functioning correctly. The added type definitions also enhance type safety as expected.
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/models.js (2)
9-15: Great improvement in type safety!The change from
anytonumbertype enhances type safety for log levels.
17-26:Details
✅ Verification successful
LGTM! Verify auto-generation.
The predefined log level constants follow a consistent pattern with 4-unit increments, aligning with standard logging practices where higher values indicate more severe levels.
Run the following script to verify the auto-generation process:
🏁 Script executed:
#!/bin/bash # Description: Verify that the file is auto-generated correctly and check for similar patterns in other service models. # Test: Check if there are any other auto-generated model files with similar patterns fd -e js -x grep -l "This file is automatically generated" {} | grep "models.js"Length of output: 17377
LGTM. Auto-generation Verified
The predefined log level constants (Debug: -4, Info: 0, Warning: 4, Error: 8) follow the expected 4-unit increment pattern, consistent with standard logging practices. The verification script confirms that this file is auto-generated—as seen by its presence alongside many similar auto-generated model files—ensuring consistency across the codebase.
- File:
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/models.js- Verification: Auto-generation markers are detected in this file, matching the pattern used in other service model files.
v3/examples/services/main.go (2)
5-8: LGTM!The added imports are necessary for the enhanced functionality:
log/slogfor logging,osfor system operations,path/filepathfor path manipulation, andruntimefor runtime information.
39-41: LGTM! Service initialization changes align with standardization goals.The changes consistently use
NewWithConfigacross all services, improving API consistency. The configuration updates (e.g.,DBFile→DBSource) reflect the enhanced service APIs.Also applies to: 42-45, 47-51
v3/pkg/services/sqlite/stmt.js (4)
1-8: LGTM! Well-structured TypeScript setup.The TypeScript configuration and symbol declarations provide proper type safety and encapsulation.
10-17: LGTM! Clear and comprehensive documentation.The class documentation clearly explains the purpose, concurrency support, and resource management requirements.
25-46: LGTM! Well-implemented constructor with proper encapsulation.The constructor properly initializes private members using symbols and includes comprehensive type annotations.
53-78: LGTM! Robust method implementations.The methods are well-implemented with:
- Support for early cancellation
- Proper type annotations
- Clear documentation
- Consistent error handling
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/stmt.js (1)
1-79: Duplicate file - no additional review needed.This file is identical to
v3/pkg/services/sqlite/stmt.jsand appears to be auto-generated bindings.v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/service.js (1)
13-16: LGTM! Well-implemented service operations.The functions are well-implemented with:
- Clear documentation
- Proper type annotations
- Support for cancellation
- Consistent error handling
Also applies to: 23-26, 33-36, 45-48, 55-58
v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/service.js (7)
19-22: LGTM!The function is well-documented with JSDoc comments and correctly implements the Debug level logging functionality.
30-33: LGTM!The function is well-documented with JSDoc comments and correctly implements the Error level logging functionality.
41-44: LGTM!The function is well-documented with JSDoc comments and correctly implements the Info level logging functionality.
64-67: LGTM!The function is well-documented with JSDoc comments and correctly implements the dynamic level logging functionality.
75-78: LGTM!The function is well-documented with JSDoc comments and correctly implements the log level retrieval functionality.
85-88: LGTM!The function is well-documented with JSDoc comments and correctly implements the log level modification functionality.
96-99: LGTM!The function is well-documented with JSDoc comments and correctly implements the Warning level logging functionality.
v3/examples/services/assets/index.html (2)
9-12: LGTM!The import statements have been correctly updated to use the standardized
Serviceexport from each service module.
44-44: LGTM!The method calls have been correctly updated to use the new standardized method names and service interfaces.
Also applies to: 149-159
docs/src/content/docs/changelog.mdx (4)
30-32: LGTM! Breaking changes are well documented.The breaking changes section clearly documents the service method renames and relocations, which align with the PR objectives of standardizing the API.
55-65: LGTM! Service enhancements are well documented.The additions section effectively documents the new service features:
- Configure method for dynamic reconfiguration
- Default behaviors for unconfigured services
- New service methods and their purposes
67-91: LGTM! Bug fixes are well documented.The fixes section comprehensively documents improvements to binding generation and model file handling, which are crucial for service reliability.
🧰 Tools
🪛 LanguageTool
[grammar] ~70-~70: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...(IN_WINDOWS)
104-106: LGTM! Service standardization changes are well documented.The changes section effectively documents the standardization efforts:
- Consistent Service type naming
- Standardized NewWithConfig function naming
- Improved method naming for better API clarity
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/src/content/docs/tutorials/01-creating-a-service.mdx (3)
492-513: Add input validation to prevent invalid QR code generation attempts.The
QRServicestruct and its constructor could benefit from input validation to ensure proper usage.Consider adding size validation in the constructor and methods:
// QRService handles QR code generation type QRService struct { route string + minSize int + maxSize int } // NewQRService creates a new QR service func NewQRService() *QRService { - return &QRService{} + return &QRService{ + minSize: 128, // Minimum QR code size + maxSize: 1024, // Maximum QR code size + } }
582-605: Enhance error handling and user feedback.The frontend implementation could benefit from better error handling and loading state management.
Consider these improvements:
async function generateQR() { const text = document.getElementById('text').value; if (!text) { alert('Please enter some text'); return; } const img = document.getElementById('qrcode'); + const button = document.getElementById('generateButton'); + + // Disable button and show loading state + button.disabled = true; + img.src = 'loading-spinner.gif'; // Add a loading spinner image + try { // Invoke the URL method to obtain an URL for the given text. img.src = await QRService.URL(text, 256); } catch (error) { + // Show user-friendly error message + img.src = ''; + alert('Failed to generate QR code. Please try again with different text.'); + console.error('QR generation failed:', error); } finally { + button.disabled = false; } }
611-616: Add more details about error handling best practices.While the note about error handling is helpful, it could be expanded to cover more scenarios and best practices.
Consider adding examples of:
- Different types of errors that might occur
- How to handle them gracefully in the frontend
- How to provide meaningful error messages to users
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/tutorials/01-creating-a-service.mdx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (1)
docs/src/content/docs/tutorials/01-creating-a-service.mdx (1)
469-489: LGTM! Clear explanation of dynamic configuration support.The documentation effectively explains the dynamic configuration approach, which aligns well with the PR's standardization goals.
# Conflicts: # docs/src/content/docs/changelog.mdx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/src/content/docs/learn/services.mdx (2)
162-164: New Config Structure Added
The newConfigstruct with theRootPathfield is clear and concise. However, consider documenting or validating the case whenRootPathis empty since the PR summary mentions that the service should return a 503 response when unconfigured.
170-174: Renamed Constructor: NewWithConfig
TheNewWithConfigfunction correctly uses the provided configuration to initialize the file server. As mentioned earlier, consider adding error handling (or at least a note) for cases whereconfig.RootPathmight be empty so that a proper 503 response can be returned in such scenarios.docs/src/content/docs/changelog.mdx (1)
63-73: Detailed New Feature Additions
The added changelog items from lines 63 to 73 comprehensively document new features and improvements including:
- The addition of the
Configuremethod for dynamic reconfiguration.- The fileserver service now returning a 503 when unconfigured.
- The updates to the kvstore (in-memory default,
LoadandClearmethods), log service (LevelandLog), and sqlite service (in-memory default,Close, cancellation support, and prepared statement support).These entries are clear; however, consider double-checking that the descriptions fully capture the runtime behavior changes (for instance, the handling of empty configurations in the fileserver) and that the wording consistently aligns with usage in the implementation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/changelog.mdx(2 hunks)docs/src/content/docs/learn/services.mdx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests (macos-latest, 1.24)
- GitHub Check: Run Go Tests (windows-latest, 1.24)
🔇 Additional comments (4)
docs/src/content/docs/learn/services.mdx (3)
166-168: Service Struct Simplification
The updatedServicestruct now only holds thefs http.Handler, which simplifies its design. Ensure that any additional context (such as default handling when no root is provided) is covered elsewhere in the documentation.
176-178: ServiceName Method
Returning a fully qualified service name here helps with clear identification and logging. This implementation is straightforward and aligns well with the overall API standardization.
180-182: ServeHTTP Delegation
TheServeHTTPmethod simply delegates HTTP requests to the internal file server handler. This is a clean and effective design.docs/src/content/docs/changelog.mdx (1)
123-125: Standardizing Naming Conventions
The renaming of built-in service types and constructor functions – changing toServiceandNewWithConfigrespectively, as well as renaming theSelectmethod toQueryon the sqlite service – is clearly documented here. This aligns with the objective to standardize the API and should help avoid confusion.
# Conflicts: # docs/src/content/docs/changelog.mdx # v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/kvstore/keyvaluestore.js # v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/log/loggerservice.js # v3/examples/services/assets/bindings/github.com/wailsapp/wails/v3/pkg/services/sqlite/service.js
…-api [v3] Built-in service standardisation and enhancement
2 participants
Description
This PR proposes some changes meant to standardise and enhance the API of built-in services (fileserver, kvstore, log, sqlite), partly along the lines of the discussions that took place on Discord here and here.
The new API is mostly compatible with the existing one, but has some minor breaking changes.
All services now adhere to the following conventions:
Servicefor consistent JS imports (fileserver and sqlite used this convention already).New()function initialises the service with a sane default configuration (breaking).NewWithConfig(config *Config)allows consumers to provide a custom initial configuration.Service.Configure(config *Config)allows late configuration and dynamic reconfiguration.Service.Configureis never exposed to the frontend, because it could allow arbitrary accesses to the filesystem.fileserver
nilconfig => no root specified) responds to all requests with 503 Service Unavailable.kvstore
nilconfig => no filename specified) provides an in-memory store.LoadandSavemethods have no effect.Service.Load()replaces the current content of the store with those of the associated file.Service.Save()writes the current content of the store to the associated file.Service.Clear()empties the store.On-disk stores are always loaded at startup and saved at shutdown. A clarifying comment has been added to the
AutoSaveoption.What if one wishes to load from disk, but then keep in-memory only or write to another file? There you go:
Notice how
Service.Configurechanges the configuration but does not alter the state otherwise, i.e. all data is kept, no automatic loads or saves are performed.log
nil) writes toapplication.DefaultLogger(slog.LevelInfo)log.Levelpublishes Debug/Info/Warning/Error constants to JSService.Log(ctx context.Context, level Level, message string, args ...any)logs with dynamic level. When called from JS, it receives the binding call context that links to the source window. Custom handlers may access this value.Service.DebugContext,Service.InfoContextetc., matching theslogAPI, accept a context; they are exported to js asDebug,Infoetc and receive the binding call context.The logger implements the
slog.Levelerinterface to provide dynamic level configuration to custom handlers. See doc comment of theService.Levelmethod for an example.sqlite
nil) creates an in-memory database.Service.Configuremethod changes the configuration but does not close or reopen the DB.Service.Open()opens (if closed) or reopens the DB, applying the latest configuration.Service.Close()closes the DB if open; has no effect if closed.Service.Select(...)renamed toService.Query(...)for consistency withsql.DB.Service.ExecContext(...)andService.QueryContext(...)accept a context for cancellation support. These are exported to JS asExecandQuery, hence cancellation is always supported from JS.Service.Prepare(query string)(cancelable versionService.PrepareContext) creates prepared statements.Normally statements must be closed manually, but they are closed automatically upon calling
Service.Close(). Unlike regularsql.Stmts, theirClosemethod is idempotent.Advanced binding generator features are used to bring prepared statements to the JS side!
Type of change
How Has This Been Tested?
Ran the services example and verified everything's working fine.
Test Configuration
Checklist:
docs/src/content/docs/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Configuremethod.Stmtclass for managing prepared statements in the SQLite service.Load,Clear, andDelete.SelecttoQuery.Bug Fixes
Documentation
Refactor