Merged
Conversation
📝 WalkthroughWalkthroughAdds a permission subsystem: domain models and origin normalization, repository port and SQLite/sqlc persistence (migration + lazy wrapper), a permission use case with logger boundary, WebKit permission signal handling and injector shim, UI dialog/indicator components, dependency wiring, generated mocks, tests, and WebRTC diagnostics page and assets. Changes
Sequence Diagram(s)sequenceDiagram
participant WebKit as WebKit
participant Content as ContentCoordinator
participant UseCase as HandlePermissionUseCase
participant Dialog as PermissionDialogPresenter
participant Repo as PermissionRepository
Note over WebKit,Content: page issues permission request
WebKit->>Content: OnPermissionRequest(origin, permKinds, allow, deny)
Content->>UseCase: HandlePermissionRequest(ctx, origin, permTypes, callback)
alt auto-allow or stored decision
UseCase->>Repo: Get / GetAll (origin, permType)
Repo-->>UseCase: stored decision(s)
UseCase-->>Content: callback.Allow() / callback.Deny()
else prompt required
UseCase->>Dialog: ShowPermissionDialog(ctx, origin, permTypes, userCallback)
Dialog-->>UseCase: user -> (Allowed, Persistent)
alt Persistent selected
UseCase->>Repo: Set(ctx, PermissionRecord) for persistable types
Repo-->>UseCase: ack
end
UseCase-->>Content: callback.Allow() / callback.Deny()
end
Content-->>WebKit: invoke allow()/deny() on original request
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
Comment |
Greptile OverviewGreptile SummaryThis PR implements a complete, well-architected media permission system following clean architecture principles. The implementation adds support for WebKit permission requests (microphone, camera, display capture, device enumeration) with W3C-compliant persistence rules. Key Changes
Architecture ComplianceThe implementation strictly follows the clean architecture patterns outlined in
Test Coverage
Security & Compliance
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Site as Website
participant WK as WebKit WebView
participant UC as HandlePermissionUseCase
participant Repo as PermissionRepository
participant Dialog as PermissionDialog
participant User as User
Site->>WK: getUserMedia() request
WK->>WK: Extract origin from URI
WK->>WK: Determine permission types
WK->>WK: Ref() request object
WK->>UC: HandlePermissionRequest(origin, types, allow, deny)
alt Display/DeviceInfo (auto-allow)
UC->>UC: isAutoAllowSet() = true
UC->>WK: callback.Allow()
WK->>Site: Permission granted
else Mic/Camera
UC->>Repo: Get(origin, type)
alt Permission stored (granted)
Repo-->>UC: PermissionRecord{Decision: granted}
UC->>WK: callback.Allow()
WK->>Site: Permission granted
else Permission stored (denied)
Repo-->>UC: PermissionRecord{Decision: denied}
UC->>WK: callback.Deny()
WK->>Site: Permission denied
else No stored permission
Repo-->>UC: nil
UC->>Dialog: ShowPermissionDialog(origin, types, callback)
Dialog->>User: Show AlertDialog
alt User clicks "Allow"
User->>Dialog: Allow (once)
Dialog->>UC: callback{Allowed: true, Persistent: false}
UC->>WK: callback.Allow()
WK->>WK: Unref() request object
WK->>Site: Permission granted
else User clicks "Always Allow"
User->>Dialog: Always Allow
Dialog->>UC: callback{Allowed: true, Persistent: true}
UC->>WK: callback.Allow()
UC->>Repo: Set(PermissionRecord{Decision: granted})
WK->>WK: Unref() request object
WK->>Site: Permission granted
else User clicks "Deny" or closes dialog
User->>Dialog: Deny (once)
Dialog->>UC: callback{Allowed: false, Persistent: false}
UC->>WK: callback.Deny()
WK->>WK: Unref() request object
WK->>Site: Permission denied
else User clicks "Always Deny"
User->>Dialog: Always Deny
Dialog->>UC: callback{Allowed: false, Persistent: true}
UC->>WK: callback.Deny()
UC->>Repo: Set(PermissionRecord{Decision: denied})
WK->>WK: Unref() request object
WK->>Site: Permission denied
end
end
end
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a full-stack media permission system for the browser, covering domain entities, persistence, WebKit integration, application logic, and UI dialogs for microphone, camera, display capture, and device info.
Changes:
- Adds domain entities (
PermissionType,PermissionDecision,PermissionRecord) plus repository interface, along with persistence in SQLite (migration, sqlc queries, repositories, lazy repos). - Integrates WebKit’s permission-request signal with a new
HandlePermissionUseCase, including auto-allow logic, stored decisions, and a UI-backed dialog flow viaPermissionDialogPresenter. - Wires the permission system into the main application and UI coordinators, and adds comprehensive unit tests for both the domain entities and the permission use case.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/ui/dialog/permission.go |
Implements the Adwaita-based permission dialog presenter that the use case uses to prompt users for mic/camera access decisions. |
internal/ui/deps.go |
Extends UI dependencies to include the new PermissionRepository and HandlePermissionUseCase so they can be injected into UI components. |
internal/ui/coordinator/content.go |
Wires WebView permission requests into HandlePermissionUseCase, translating WebKit permission strings into entity.PermissionType and deciding allow/deny behavior. |
internal/ui/app.go |
Constructs the permission use case and dialog, injects them into the UI layer, and passes PermissionUC into the content coordinator. |
internal/infrastructure/webkit/webview.go |
Adds WebKit permission-request signal handling, origin extraction, permission type detection, and exposes an OnPermissionRequest callback used by higher layers (note: see inline comments about GObject ref/unref safety). |
internal/infrastructure/persistence/sqlite/sqlc/querier.go |
Extends the generated Querier interface with CRUD methods for permissions, enabling the SQLite permission repository to use sqlc. |
internal/infrastructure/persistence/sqlite/sqlc/permissions.sql.go |
Generated sqlc implementation of the permissions queries (get, set, delete, list), backing the new permission repository. |
internal/infrastructure/persistence/sqlite/sqlc/models.go |
Introduces the generated Permission model struct mapping the permissions table schema. |
internal/infrastructure/persistence/sqlite/queries/permissions.sql |
Defines the SQL for the permissions CRUD operations and listings used by sqlc. |
internal/infrastructure/persistence/sqlite/permission_repo.go |
Implements repository.PermissionRepository on top of sqlc, including logging and mapping between SQL rows and domain PermissionRecord. |
internal/infrastructure/persistence/sqlite/migrations/007_permissions.sql |
Adds the permissions table and indexes, enforcing valid decision values and establishing (origin, permission_type) as the primary key. |
internal/infrastructure/persistence/sqlite/lazy_repos.go |
Adds a lazy-loading PermissionRepository wrapper, aligning permission persistence with the existing lazy repo infrastructure. |
internal/domain/repository/permission.go |
Defines the PermissionRepository interface for domain-level permission persistence (get/set/delete/list). |
internal/domain/repository/mocks/mocks.go |
Generates a MockPermissionRepository for use in tests, supporting all PermissionRepository methods. |
internal/domain/entity/permission_test.go |
Tests permission type/decision constants and business rules (CanPersist, IsAutoAllow, PermissionRecord helpers, and PermissionSet). |
internal/domain/entity/permission.go |
Introduces the permission domain model, including permission types, decisions, persistence/auto-allow rules, and logging helpers. |
internal/application/usecase/handle_permission_test.go |
Provides comprehensive tests for HandlePermissionUseCase (auto-allow logic, stored decisions, dialog flows, persistence, origin extraction). |
internal/application/usecase/handle_permission.go |
Implements the core permission handling use case, coordinating auto-allow, repository lookups, dialog presentation, and persistence of “Always” decisions. |
internal/application/port/webview.go |
Extends WebViewCallbacks with an OnPermissionRequest callback signature for application-level permission handling. |
internal/application/port/permission.go |
Defines PermissionDialogResult and the PermissionDialogPresenter interface used to abstract UI dialogs from the use case. |
internal/application/port/mocks/mocks.go |
Adds mocks for DownloadResponse and PermissionDialogPresenter, supporting tests for permission and download flows. |
cmd/dumber/main.go |
Wires the new permission repository and use case into the application bootstrap and UI dependency graph. |
AGENTS.md |
Documents recommended pre-commit commands (linting, tests, checks, formatting) for contributors. |
.mockery.yaml |
Configures mockery to generate mocks for the new permission-related interfaces (PermissionDialogPresenter, PermissionRepository). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+896
to
+904
| // Ref the request object to prevent use-after-free | ||
| // The request may outlive the signal handler if we show a dialog | ||
| requestObj := gobject.ObjectNewFromInternalPtr(requestPtr) | ||
| if requestObj == nil { | ||
| wv.logger.Warn().Msg("permission request: failed to wrap request object") | ||
| return false | ||
| } | ||
| requestObj.Ref() | ||
|
|
There was a problem hiding this comment.
requestObj.Ref() is called here but there is no guaranteed matching Unref when the handler returns false from OnPermissionRequest without invoking allow or deny, which will leak the GObject reference. To avoid leaking the permission request object, ensure you unref it on all exit paths (e.g., unref before returning when the handler reports the request as unhandled, or only ref when the handler is going to take ownership and definitely call one of the callbacks).
| ) | ||
|
|
||
| // PermissionRepository defines operations for permission persistence. | ||
| // Only microphone and camera permissions are persisted per W3C spec. |
There was a problem hiding this comment.
The comment says "Only microphone and camera permissions are persisted per W3C spec", but entity.CanPersist currently returns true for several other permission types (clipboard, notification, geolocation, etc.), so the implementation allows persisting more than just mic/camera. To avoid confusion for future changes, please update this comment to reflect the actual persistence policy or adjust CanPersist so the code and documentation match.
Suggested change
| // Only microphone and camera permissions are persisted per W3C spec. | |
| // Only permissions that are allowed to be persisted (see entity.CanPersist) are stored. |
Comment on lines
+939
to
+946
| var types []string | ||
|
|
||
| // Try to cast to UserMediaPermissionRequest | ||
| userMediaReq := webkit.UserMediaPermissionRequestNewFromInternalPtr(requestPtr) | ||
| if userMediaReq != nil { | ||
| if webkit.UserMediaPermissionIsForAudioDevice(userMediaReq) { | ||
| types = append(types, "microphone") | ||
| } |
There was a problem hiding this comment.
extractOrigin here duplicates the logic of ExtractOrigin in internal/application/usecase/handle_permission.go, which also normalizes URIs to scheme://host. To reduce the risk of these diverging over time, consider moving this helper into a shared package (e.g., a domain/url utility) and reusing it from both places instead of maintaining two independent implementations.
Suggested change
| var types []string | |
| // Try to cast to UserMediaPermissionRequest | |
| userMediaReq := webkit.UserMediaPermissionRequestNewFromInternalPtr(requestPtr) | |
| if userMediaReq != nil { | |
| if webkit.UserMediaPermissionIsForAudioDevice(userMediaReq) { | |
| types = append(types, "microphone") | |
| } | |
| return urlutil.ExtractOrigin(uri) |
Comment on lines
+909
to
+917
| allow := func() { | ||
| if allowCalled || denyCalled { | ||
| return | ||
| } | ||
| allowCalled = true | ||
| wv.allowPermissionRequest(requestPtr) | ||
| requestObj.Unref() | ||
| } | ||
|
|
There was a problem hiding this comment.
In allow/deny you unref the requestObj both in the "already handled" early-return path and after calling allowPermissionRequest/denyPermissionRequest. This means that if client code accidentally calls allow or deny more than once, the second call will still execute requestObj.Unref(), leading to a double-unref/use-after-free. To make this safe, avoid calling Unref in the allowCalled || denyCalled branch and only unref exactly once when transitioning from the unhandled to handled state.
![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
🤖 Fix all issues with AI agents
In `@docs/reference/permissions.md`:
- Around line 27-33: Fix the markdown table spacing so it satisfies markdownlint
MD060 by ensuring a single space on both sides of each pipe and consistent
column alignment across all rows; update the header row "Button | Action |
Persistence" and each corresponding row ("**Allow** | Grant permission once |
Not saved", "**Always Allow** | Grant and remember | Saved per origin",
"**Deny** | Block permission once | Not saved", "**Always Deny** | Block and
remember | Saved per origin") to use consistent single-space padding around
pipes and remove any trailing or extra spaces.
In `@internal/application/usecase/handle_permission.go`:
- Around line 58-109: Handle the case where permTypes is empty by not treating
it as auto-allow: update the logic in HandlePermissionRequest (or isAutoAllowSet
usage) so that when len(permTypes) == 0 you either call callback.Deny() (or
treat as entity.PermissionPrompt) instead of relying on isAutoAllowSet;
specifically, before calling isAutoAllowSet(permTypes) add a guard that
denies/prompts for empty slices (or change isAutoAllowSet to return false for
empty input), and keep the subsequent flows that call uc.checkStoredPermissions
and uc.showPermissionDialog unchanged.
- Around line 156-209: checkStoredPermissions currently skips types where
entity.CanPersist(permType) is false which lets non‑persistable, non‑auto‑allow
permissions fall through and potentially cause a PermissionGranted; change the
logic in checkStoredPermissions so that only entity.IsAutoAllow(permType) is
continued, but if !entity.CanPersist(permType) treat it as unknown by setting
hasPrompt = true (do not continue silently), keep the existing permRepo.Get
handling and the record.Decision checks (PermissionDenied/PermissionPrompt)
unchanged so non‑persistable types force a PermissionPrompt rather than being
implicitly granted.
In `@internal/domain/entity/permission.go`:
- Around line 3-62: The domain entity PermissionRecord currently imports time
and uses time.Time for UpdatedAt which violates the rule against standard
library/infrastructure imports in domain; change PermissionRecord.UpdatedAt to a
domain-owned primitive type (e.g., DomainTimestamp or string/int64) declared in
this package instead of time.Time, remove the time import, and update any
constructors/serializers in this package to use that domain type; then update
infrastructure/adapters to map between the new DomainTimestamp and time.Time
when persisting/reading (ensure the new type preserves needed semantics and
zero-value behavior).
In `@internal/domain/url/normalize_test.go`:
- Around line 513-568: Update the ExtractOrigin tests to cover default-port
normalization and host-casing normalization: in the TestExtractOrigin_ValidURI
table (or a new table within the same test) add cases where URIs include
explicit default ports (e.g., "https://example.com:443/path" expecting
"https://example.com" and "http://example.com:80/path" expecting
"http://example.com") and a case with an uppercase host (e.g.,
"https://EXAMPLE.COM/path" expecting "https://example.com"); ensure these cases
call ExtractOrigin and assert the returned origin matches the normalized
expectation so the behavior of ExtractOrigin (the ExtractOrigin function) is
locked in.
In `@internal/domain/url/normalize.go`:
- Around line 207-221: The ExtractOrigin function currently returns
parsed.Scheme + "://" + parsed.Host which preserves host case and explicit
default ports; update ExtractOrigin to canonicalize by lowercasing parsed.Scheme
and parsed.Hostname(), use parsed.Port() to determine whether to include a port,
and omit the port when it equals the scheme default (80 for http, 443 for
https); construct the origin as lowercaseScheme + "://" + lowercaseHostname
(append ":"+port only if port is non-empty and not the default for the scheme)
so origins like "https://EXAMPLE.com:443" and "https://example.com" normalize to
the same value.
In `@internal/infrastructure/persistence/sqlite/permission_repo.go`:
- Around line 41-53: In permissionRepo.Set, guard against a nil record before
any dereference: check if record == nil at the start of Set (before calling
logging.FromContext().Debug() or building sqlc.SetPermissionParams) and return a
clear error (e.g., an errors.New or fmt.Errorf) so the method does not panic;
retain logging.FromContext(ctx) and r.queries.SetPermission usage after the
nil-check and optionally log the nil-input error with the same logger for
consistency.
In `@internal/infrastructure/webkit/webview.go`:
- Around line 871-929: The permission request handler currently Ref()s
requestObj but only Unref()s inside the allow/deny closures, leaking when the
user handler returns false (unhandled); in
WebView.connectPermissionRequestSignal's permissionCb ensure that after calling
wv.OnPermissionRequest(origin, permTypes, allow, deny) you Unref() requestObj if
the callback returned false and neither allowCalled nor denyCalled fired, taking
care not to Unref() twice (i.e., only Unref in the unhandled path), so
requestObj is always balanced regardless of the handler's return value.
Comment on lines
+871
to
+929
| func (wv *WebView) connectPermissionRequestSignal() { | ||
| permissionCb := func(_ webkit.WebView, requestPtr uintptr) bool { | ||
| if wv.OnPermissionRequest == nil { | ||
| return false // Not handled, WebKit will deny by default | ||
| } | ||
|
|
||
| // Extract and normalize origin from current URI | ||
| uri := wv.URI() | ||
| if uri == "" { | ||
| wv.logger.Debug().Msg("permission request with empty origin, denying") | ||
| return false | ||
| } | ||
| origin, err := urlutil.ExtractOrigin(uri) | ||
| if err != nil { | ||
| wv.logger.Debug().Str("uri", uri).Err(err).Msg("permission request: failed to extract origin, denying") | ||
| return false | ||
| } | ||
|
|
||
| // Determine permission types from the request | ||
| permTypes := wv.determinePermissionTypes(requestPtr) | ||
| if len(permTypes) == 0 { | ||
| wv.logger.Warn().Msg("permission request with unknown type, denying") | ||
| return false | ||
| } | ||
|
|
||
| // Ref the request object to prevent use-after-free | ||
| // The request may outlive the signal handler if we show a dialog | ||
| requestObj := gobject.ObjectNewFromInternalPtr(requestPtr) | ||
| if requestObj == nil { | ||
| wv.logger.Warn().Msg("permission request: failed to wrap request object") | ||
| return false | ||
| } | ||
| requestObj.Ref() | ||
|
|
||
| // Create allow/deny callbacks that wrap the WebKit permission request | ||
| allowCalled := false | ||
| denyCalled := false | ||
|
|
||
| allow := func() { | ||
| if allowCalled || denyCalled { | ||
| return | ||
| } | ||
| allowCalled = true | ||
| wv.allowPermissionRequest(requestPtr) | ||
| requestObj.Unref() | ||
| } | ||
|
|
||
| deny := func() { | ||
| if allowCalled || denyCalled { | ||
| return | ||
| } | ||
| denyCalled = true | ||
| wv.denyPermissionRequest(requestPtr) | ||
| requestObj.Unref() | ||
| } | ||
|
|
||
| // Call the handler | ||
| return wv.OnPermissionRequest(origin, permTypes, allow, deny) | ||
| } |
There was a problem hiding this comment.
Avoid leaking the permission-request ref when a handler returns false.
You Ref() the request object but only Unref() inside allow/deny. If the callback returns false (unhandled), the request stays referenced and leaks. Unref on the unhandled path unless allow/deny already fired.
🛠️ Proposed fix
- // Call the handler
- return wv.OnPermissionRequest(origin, permTypes, allow, deny)
+ // Call the handler
+ handled := wv.OnPermissionRequest(origin, permTypes, allow, deny)
+ if !handled && !allowCalled && !denyCalled {
+ requestObj.Unref()
+ }
+ return handledIn puregotk/webkit, when handling WebKit permission-request callbacks, must a ref’ed GObject be unref’ed if the handler returns false (unhandled)?
🤖 Prompt for AI Agents
In `@internal/infrastructure/webkit/webview.go` around lines 871 - 929, The
permission request handler currently Ref()s requestObj but only Unref()s inside
the allow/deny closures, leaking when the user handler returns false
(unhandled); in WebView.connectPermissionRequestSignal's permissionCb ensure
that after calling wv.OnPermissionRequest(origin, permTypes, allow, deny) you
Unref() requestObj if the callback returned false and neither allowCalled nor
denyCalled fired, taking care not to Unref() twice (i.e., only Unref in the
unhandled path), so requestObj is always balanced regardless of the handler's
return value.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@internal/domain/entity/permission_test.go`:
- Around line 128-152: Update TestIsAutoAllow to include explicit cases for
media_key_system and website_data_access: add test entries with names like
"media key system" and "website data access" using
entity.PermissionTypeMediaKeySystem and entity.PermissionTypeWebsiteDataAccess
respectively, both expecting false, and ensure they are asserted via
entity.IsAutoAllow so behavior is covered and won't regress.
In `@internal/domain/entity/permission.go`:
- Around line 3-5: The top-of-file comment referencing a non-existent
"DomainTimestamp" should be fixed: either remove the orphaned comment entirely
or update it to describe the actual implementation (that UpdatedAt is stored as
an int64 Unix timestamp in seconds). Locate the comment near the top of
internal/domain/entity/permission.go and either delete the lines mentioning
DomainTimestamp or replace them with a brief note that UpdatedAt is an int64
Unix timestamp (and remove any mention of a domain-owned DomainTimestamp type).
In `@internal/domain/url/normalize_test.go`:
- Around line 513-578: Add a test case in TestExtractOrigin_ValidURI to cover
IPv6 bracketed-origin behavior: include a URI with an IPv6 literal (e.g.,
"http://[2001:db8::1]:8080/path" and a similar default-port case like
"http://[2001:db8::1]:80/path") and assert ExtractOrigin returns the origin with
the host in brackets and port omitted when default (e.g.,
"http://[2001:db8::1]:8080" and "http://[2001:db8::1]" respectively); locate the
test table in TestExtractOrigin_ValidURI and add entries so ExtractOrigin is
verified to preserve brackets and correctly handle default vs non-default ports.
In `@internal/ui/component/permission_popup.go`:
- Around line 138-243: createWidgets is too large (73 statements); split it into
small helper methods to reduce statement count by moving logical blocks into
functions like initOuterBox (creates pp.outerBox and styles), initMainBox
(creates pp.mainBox), initLabels (creates pp.headingLabel and pp.bodyLabel),
initButtons (creates pp.btnAlwaysDeny, pp.btnDeny, pp.btnAllow,
pp.btnAlwaysAllow and styles them), wireButtonCallbacks (creates retained
callbacks that call pp.dismiss and connects them), and assembleLayout (appends
widgets into btnRow, pp.mainBox and pp.outerBox). Have createWidgets simply call
each helper in sequence and propagate/return any error; keep all references to
the existing symbols (PermissionPopup, createWidgets, pp.outerBox, pp.mainBox,
pp.headingLabel, pp.bodyLabel, pp.btnAlwaysDeny, pp.btnDeny, pp.btnAllow,
pp.btnAlwaysAllow, pp.retainedCallbacks, pp.dismiss) so callers remain
unchanged.
- Line 175: Extract the literal spacing value used when creating the button row
into a named constant (e.g., btnRowSpacing or buttonSpacing) and replace the
magic number in the call to gtk.NewBox inside the permission popup code where
btnRow is created (the line with btnRow :=
gtk.NewBox(gtk.OrientationHorizontalValue, 6)); define the constant near the top
of the file or the function scope so it is clearly named and documented, then
use that constant in place of 6 to satisfy the linter and improve readability.
In `@internal/ui/dialog/permission.go`:
- Around line 62-115: Both buildHeading and buildBody duplicate the same loop to
detect microphone/camera presence; extract that logic into a helper on
PermissionDialog (e.g., a method like detectPerms or hasPerms that takes
[]entity.PermissionType and returns (hasMic, hasCam) bools) and call it from
buildHeading and buildBody, replacing their per-function loops and leaving the
existing switch/formatting logic intact (use entity.PermissionTypeMicrophone and
entity.PermissionTypeCamera in the helper).
Comment on lines
+128
to
+152
| func TestIsAutoAllow(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| permType entity.PermissionType | ||
| expected bool | ||
| }{ | ||
| // Auto-allowed (portal handles UI or low risk) | ||
| {"display capture", entity.PermissionTypeDisplay, true}, | ||
| {"device info", entity.PermissionTypeDeviceInfo, true}, | ||
| {"pointer lock", entity.PermissionTypePointerLock, true}, | ||
|
|
||
| // Not auto-allowed (require user consent) | ||
| {"microphone", entity.PermissionTypeMicrophone, false}, | ||
| {"camera", entity.PermissionTypeCamera, false}, | ||
| {"clipboard", entity.PermissionTypeClipboard, false}, | ||
| {"notification", entity.PermissionTypeNotification, false}, | ||
| {"geolocation", entity.PermissionTypeGeolocation, false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.expected, entity.IsAutoAllow(tt.permType)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test cases for media_key_system and website_data_access.
TestIsAutoAllow doesn't cover PermissionTypeMediaKeySystem and PermissionTypeWebsiteDataAccess. While they correctly fall into the default case (returning false), adding explicit test cases would ensure the behavior is documented and won't regress if the implementation changes.
🧪 Suggested additions to test cases
// Not auto-allowed (require user consent)
{"microphone", entity.PermissionTypeMicrophone, false},
{"camera", entity.PermissionTypeCamera, false},
{"clipboard", entity.PermissionTypeClipboard, false},
{"notification", entity.PermissionTypeNotification, false},
{"geolocation", entity.PermissionTypeGeolocation, false},
+ {"media key system", entity.PermissionTypeMediaKeySystem, false},
+ {"website data access", entity.PermissionTypeWebsiteDataAccess, false},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestIsAutoAllow(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| permType entity.PermissionType | |
| expected bool | |
| }{ | |
| // Auto-allowed (portal handles UI or low risk) | |
| {"display capture", entity.PermissionTypeDisplay, true}, | |
| {"device info", entity.PermissionTypeDeviceInfo, true}, | |
| {"pointer lock", entity.PermissionTypePointerLock, true}, | |
| // Not auto-allowed (require user consent) | |
| {"microphone", entity.PermissionTypeMicrophone, false}, | |
| {"camera", entity.PermissionTypeCamera, false}, | |
| {"clipboard", entity.PermissionTypeClipboard, false}, | |
| {"notification", entity.PermissionTypeNotification, false}, | |
| {"geolocation", entity.PermissionTypeGeolocation, false}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| assert.Equal(t, tt.expected, entity.IsAutoAllow(tt.permType)) | |
| }) | |
| } | |
| } | |
| func TestIsAutoAllow(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| permType entity.PermissionType | |
| expected bool | |
| }{ | |
| // Auto-allowed (portal handles UI or low risk) | |
| {"display capture", entity.PermissionTypeDisplay, true}, | |
| {"device info", entity.PermissionTypeDeviceInfo, true}, | |
| {"pointer lock", entity.PermissionTypePointerLock, true}, | |
| // Not auto-allowed (require user consent) | |
| {"microphone", entity.PermissionTypeMicrophone, false}, | |
| {"camera", entity.PermissionTypeCamera, false}, | |
| {"clipboard", entity.PermissionTypeClipboard, false}, | |
| {"notification", entity.PermissionTypeNotification, false}, | |
| {"geolocation", entity.PermissionTypeGeolocation, false}, | |
| {"media key system", entity.PermissionTypeMediaKeySystem, false}, | |
| {"website data access", entity.PermissionTypeWebsiteDataAccess, false}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| assert.Equal(t, tt.expected, entity.IsAutoAllow(tt.permType)) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/domain/entity/permission_test.go` around lines 128 - 152, Update
TestIsAutoAllow to include explicit cases for media_key_system and
website_data_access: add test entries with names like "media key system" and
"website data access" using entity.PermissionTypeMediaKeySystem and
entity.PermissionTypeWebsiteDataAccess respectively, both expecting false, and
ensure they are asserted via entity.IsAutoAllow so behavior is covered and won't
regress.
Comment on lines
+3
to
+5
| // DomainTimestamp represents a Unix timestamp in seconds. | ||
| // This is a domain-owned type to avoid importing time in the domain layer. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove or correct orphaned comment.
The comment mentions "DomainTimestamp" type but no such type is defined. The UpdatedAt field uses int64 directly. Either remove the comment or update it to reflect the actual implementation.
📝 Proposed fix
package entity
-// DomainTimestamp represents a Unix timestamp in seconds.
-// This is a domain-owned type to avoid importing time in the domain layer.
-
// PermissionType represents the type of permission being requested.
type PermissionType string🤖 Prompt for AI Agents
In `@internal/domain/entity/permission.go` around lines 3 - 5, The top-of-file
comment referencing a non-existent "DomainTimestamp" should be fixed: either
remove the orphaned comment entirely or update it to describe the actual
implementation (that UpdatedAt is stored as an int64 Unix timestamp in seconds).
Locate the comment near the top of internal/domain/entity/permission.go and
either delete the lines mentioning DomainTimestamp or replace them with a brief
note that UpdatedAt is an int64 Unix timestamp (and remove any mention of a
domain-owned DomainTimestamp type).
Comment on lines
+513
to
+578
| func TestExtractOrigin_ValidURI(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| uri string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "https with port", | ||
| uri: "https://example.com:8443/path?query=1", | ||
| expected: "https://example.com:8443", | ||
| }, | ||
| { | ||
| name: "https without port", | ||
| uri: "https://example.com/path", | ||
| expected: "https://example.com", | ||
| }, | ||
| { | ||
| name: "http", | ||
| uri: "http://localhost:8080/app", | ||
| expected: "http://localhost:8080", | ||
| }, | ||
| { | ||
| name: "uppercase host", | ||
| uri: "https://EXAMPLE.COM/path", | ||
| expected: "https://example.com", | ||
| }, | ||
| { | ||
| name: "uppercase scheme", | ||
| uri: "HTTPS://example.com/path", | ||
| expected: "https://example.com", | ||
| }, | ||
| { | ||
| name: "https default port omitted", | ||
| uri: "https://example.com:443/path", | ||
| expected: "https://example.com", | ||
| }, | ||
| { | ||
| name: "http default port omitted", | ||
| uri: "http://example.com:80/path", | ||
| expected: "http://example.com", | ||
| }, | ||
| { | ||
| name: "https explicit non-default port kept", | ||
| uri: "https://example.com:8080/path", | ||
| expected: "https://example.com:8080", | ||
| }, | ||
| { | ||
| name: "mixed case with default port", | ||
| uri: "HTTPS://EXAMPLE.COM:443/path", | ||
| expected: "https://example.com", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| origin, err := ExtractOrigin(tt.uri) | ||
| if err != nil { | ||
| t.Errorf("ExtractOrigin(%q) returned error: %v", tt.uri, err) | ||
| return | ||
| } | ||
| if origin != tt.expected { | ||
| t.Errorf("ExtractOrigin(%q) = %q, want %q", tt.uri, origin, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an IPv6 case to lock in bracketed-origin behavior.
🧪 Suggested test addition
@@
{
name: "mixed case with default port",
uri: "HTTPS://EXAMPLE.COM:443/path",
expected: "https://example.com",
},
+ {
+ name: "ipv6 literal with port",
+ uri: "https://[2001:db8::1]:8443/path",
+ expected: "https://[2001:db8::1]:8443",
+ },🤖 Prompt for AI Agents
In `@internal/domain/url/normalize_test.go` around lines 513 - 578, Add a test
case in TestExtractOrigin_ValidURI to cover IPv6 bracketed-origin behavior:
include a URI with an IPv6 literal (e.g., "http://[2001:db8::1]:8080/path" and a
similar default-port case like "http://[2001:db8::1]:80/path") and assert
ExtractOrigin returns the origin with the host in brackets and port omitted when
default (e.g., "http://[2001:db8::1]:8080" and "http://[2001:db8::1]"
respectively); locate the test table in TestExtractOrigin_ValidURI and add
entries so ExtractOrigin is verified to preserve brackets and correctly handle
default vs non-default ports.
Comment on lines
+62
to
+115
| // buildHeading creates the dialog heading based on permission types. | ||
| func (d *PermissionDialog) buildHeading(permTypes []entity.PermissionType) string { | ||
| hasMic := false | ||
| hasCam := false | ||
|
|
||
| for _, pt := range permTypes { | ||
| switch pt { | ||
| case entity.PermissionTypeMicrophone: | ||
| hasMic = true | ||
| case entity.PermissionTypeCamera: | ||
| hasCam = true | ||
| } | ||
| } | ||
|
|
||
| switch { | ||
| case hasMic && hasCam: | ||
| return "Allow Microphone and Camera?" | ||
| case hasMic: | ||
| return "Allow Microphone Access?" | ||
| case hasCam: | ||
| return "Allow Camera Access?" | ||
| default: | ||
| return "Allow Permission?" | ||
| } | ||
| } | ||
|
|
||
| // buildBody creates the dialog body text. | ||
| func (d *PermissionDialog) buildBody(origin string, permTypes []entity.PermissionType) string { | ||
| hasMic := false | ||
| hasCam := false | ||
|
|
||
| for _, pt := range permTypes { | ||
| switch pt { | ||
| case entity.PermissionTypeMicrophone: | ||
| hasMic = true | ||
| case entity.PermissionTypeCamera: | ||
| hasCam = true | ||
| } | ||
| } | ||
|
|
||
| var action string | ||
| switch { | ||
| case hasMic && hasCam: | ||
| action = "access your microphone and camera" | ||
| case hasMic: | ||
| action = "access your microphone" | ||
| case hasCam: | ||
| action = "access your camera" | ||
| default: | ||
| action = "access your device" | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s wants to %s.", origin, action) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting duplicate permission type detection.
buildHeading and buildBody both iterate over permTypes to detect microphone/camera presence. This can be extracted to reduce duplication.
♻️ Proposed refactor
+// detectPermissionTypes returns whether microphone and camera permissions are requested.
+func detectPermissionTypes(permTypes []entity.PermissionType) (hasMic, hasCam bool) {
+ for _, pt := range permTypes {
+ switch pt {
+ case entity.PermissionTypeMicrophone:
+ hasMic = true
+ case entity.PermissionTypeCamera:
+ hasCam = true
+ }
+ }
+ return
+}
+
func (d *PermissionDialog) buildHeading(permTypes []entity.PermissionType) string {
- hasMic := false
- hasCam := false
-
- for _, pt := range permTypes {
- switch pt {
- case entity.PermissionTypeMicrophone:
- hasMic = true
- case entity.PermissionTypeCamera:
- hasCam = true
- }
- }
+ hasMic, hasCam := detectPermissionTypes(permTypes)
switch {
// ...🤖 Prompt for AI Agents
In `@internal/ui/dialog/permission.go` around lines 62 - 115, Both buildHeading
and buildBody duplicate the same loop to detect microphone/camera presence;
extract that logic into a helper on PermissionDialog (e.g., a method like
detectPerms or hasPerms that takes []entity.PermissionType and returns (hasMic,
hasCam) bools) and call it from buildHeading and buildBody, replacing their
per-function loops and leaving the existing switch/formatting logic intact (use
entity.PermissionTypeMicrophone and entity.PermissionTypeCamera in the helper).
Add PermissionType, PermissionDecision, and PermissionRecord entities with W3C-compliant persistence rules (CanPersist, IsAutoAllow). Add PermissionRepository interface for storage abstraction.
Add SQLite migration for permissions table. Add SQLC queries for permission CRUD operations. Implement PermissionRepository with Get, Set, Delete, GetAll methods. Update models and querier for permission support.
Add HandlePermissionUseCase with auto-allow, stored permission checking, dialog flow, and persistence logic. Add PermissionDialogPresenter interface for UI abstraction. Add comprehensive unit tests covering all permission scenarios. Include origin extraction helper for URI normalization.
Add permission-request signal handler with g_object_ref/unref safety. Implement determinePermissionTypes to extract mic, camera, display, device_info. Add OnPermissionRequest callback to WebView port interface. Normalize origin from URI before passing to use case. Fix Destroy() to clear OnPermissionRequest callback.
Add Adwaita AlertDialog for permission requests with Allow, Always Allow, Deny, and Always Deny options. Implement PermissionDialogPresenter interface. Wire handlePermissionRequest method in ContentCoordinator.
Initialize PermissionRepository and HandlePermissionUseCase in main. Add PermissionUC and PermissionRepo to Dependencies struct. Add PermissionRepository to LazyRepositories convenience struct. Update WebView port interface with OnPermissionRequest callback. Wire permission dialog presenter when UI window is ready.
Add PermissionRepository and PermissionDialogPresenter mocks via mockery. Update .mockery.yaml with new mock targets. Add AGENTS.md commands section for pre-commit checks. Add PermissionTypesToStrings helper to entity package.
Add comprehensive documentation for the website permission system: - Explain auto-allowed vs user-confirmed permissions - Document the permission dialog options - Describe origin-based storage and W3C compliance - Add troubleshooting section - Link from reference index
- Fix race condition on dialog field with sync.RWMutex protection - Fix double Unref bug in allow/deny callbacks (remove Unref from early return) - Make isAutoAllowSet a standalone function (remove dead code suppression) - Remove unused PermissionTypeMicrophoneAndCamera constant - Move ExtractOrigin to domain/url package (shared utility) - Fix documentation error about origin sharing (github.com vs gist.github.com) - Add test for nil dialog presenter scenario - Update all references to use shared ExtractOrigin
Replace adw.AlertDialog with a custom PermissionPopup overlay component to sidestep the purego ConnectResponse bug (empty string from missing continue in syscall_sysv.go) and match the app's custom UI style. - Add PermissionPopup component following omnibox/tab picker patterns - Add themed CSS using existing variables (--accent, --destructive, etc.) - Add PermissionPopupSizeDefaults for modal sizing - Wire popup into main window overlay via app.go - Expose ContentOverlay() accessor on MainWindow - Canonicalize origins (lowercase, omit default ports) for storage - Use Unix timestamps in PermissionRecord (avoid time import in domain) - Add nil-record guard in SQLite permission repo - Handle empty permTypes defensively in use case
- Use GObject property accessors instead of C function wrappers in determinePermissionTypes to avoid purego bool return value bug - Refactor permission_popup.go to fix funlen and mnd linter issues: - Extract setupContainers() and setupLabels() helpers - Add createPermissionButton() and wireButton() helpers - Add buttonSpacing constant to replace magic number
bnema
force-pushed
the
feat/permission-system
branch
from
c4bf610 to
d037679
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/infrastructure/webkit/webview.go (1)
1680-1694:⚠️ Potential issue | 🟠 MajorClear OnPermissionRequest in ResetForPoolReuse to avoid stale callbacks.
The new callback is cleared in Destroy but not in pool reuse, which can route future requests to the wrong handler.🛠️ Suggested fix
func (wv *WebView) ResetForPoolReuse() { if wv == nil || wv.destroyed.Load() { return } wv.OnLoadChanged = nil wv.OnTitleChanged = nil wv.OnURIChanged = nil wv.OnProgressChanged = nil wv.OnFaviconChanged = nil wv.OnClose = nil wv.OnCreate = nil wv.OnReadyToShow = nil wv.OnLinkMiddleClick = nil wv.OnEnterFullscreen = nil wv.OnLeaveFullscreen = nil wv.OnAudioStateChanged = nil wv.OnLinkHover = nil wv.OnWebProcessTerminated = nil + wv.OnPermissionRequest = nil
🤖 Fix all issues with AI agents
In `@docs/reference/permissions.md`:
- Line 61: Update the "Site-specific reset" bullet that references
`navigator.permissions.revoke()` to remove or clearly mark it as
deprecated/removed from the Permissions API: replace the claim that sites can
call `navigator.permissions.revoke()` with a statement that the method was
removed and is not supported in modern browsers, and advise developers to
instruct users to use browser/site settings or account-level controls for
permission resets instead; ensure the change is made where the "Site-specific
reset" bullet appears so readers see that `navigator.permissions.revoke()` no
longer works.
In `@internal/application/usecase/handle_permission.go`:
- Around line 125-158: QueryPermissionState currently returns granted for
auto-allow types before checking stored records, which ignores manual deny
overrides; change the logic in QueryPermissionState to query
uc.permRepo.Get(ctx, origin, permType) first and if a non-nil record exists
return record.Decision (preserving the current error->PermissionPrompt
behavior), and only when record==nil fall back to checking
entity.IsAutoAllow(permType) to return PermissionGranted or PermissionPrompt
accordingly; keep the logging (log.Debug/Warn) around the repo call and final
decision and reference QueryPermissionState, uc.permRepo.Get, and
entity.IsAutoAllow when making the change.
- Around line 310-331: SetManualPermissionDecision currently writes any
permission type to storage; add a guard that uses entity.CanPersist(permType)
and return an error (or skip persistence) when CanPersist returns false so
non-persistable types (e.g., PermissionTypeDisplay, PermissionTypeDeviceInfo,
PermissionTypePointerLock) are never persisted. Specifically, at the start of
SetManualPermissionDecision check entity.CanPersist(permType) and if false
return a clear error (e.g., "permission type not persistable") instead of
calling uc.permRepo.Set or Delete; keep the existing behavior for
PermissionPrompt (delete) and persist only when CanPersist is true. Ensure
references are to SetManualPermissionDecision, entity.CanPersist,
uc.permRepo.Set and uc.permRepo.Delete.
In `@internal/domain/repository/permission.go`:
- Around line 3-7: The domain package is importing "context" in
internal/domain/repository/permission.go which breaks domain isolation; either
move the repository interface (the permission repository defined in
permission.go) to the application/port layer where importing context is allowed,
or remove the external context dependency by introducing a domain-local context
abstraction (e.g., a DomainContext type or method signatures without context)
and update all usages accordingly; ensure you reference and update the
repository interface in permission.go and any methods that currently accept
context so callers are adjusted to the chosen approach.
In `@internal/domain/url/normalize.go`:
- Around line 235-239: The origin builder can produce invalid URLs for IPv6
hostnames because parsed.Hostname() returns addresses without brackets; update
the origin construction (using the origin, hostname, port, scheme symbols) to
wrap hostname in brackets when it's an IPv6 literal: if hostname contains ':'
and does not already start with '[' then set hostname = "[" + hostname + "]"
before concatenating scheme + "://" + hostname and appending ":"+port when port
!= ""; this ensures results like "https://[::1]:8443".
In `@internal/infrastructure/webkit/gobject_type_helpers.go`:
- Around line 47-49: The helper loadGObjectTypeFns should guard against a nil
ctx before calling logging.FromContext to avoid panics; update
loadGObjectTypeFns to check if ctx == nil and if so substitute a non-nil context
(e.g. context.Background() or a dedicated fallback) before calling
logging.FromContext, ensuring the closure passed to loadGObjectTypeFnsOnce uses
the safe context when obtaining log := logging.FromContext(ctx).
- Around line 64-65: The calls to core.PuregoSafeRegister currently ignore
returned errors, leaving function pointers like typeCheckInstanceIsA and
typeNameFromInstance nil without logging; update both calls to capture the error
(if err := core.PuregoSafeRegister(&typeCheckInstanceIsA, libs,
"g_type_check_instance_is_a"); err != nil { ... }) and (if err :=
core.PuregoSafeRegister(&typeNameFromInstance, libs,
"g_type_name_from_instance"); err != nil { ... }) and log failures using
log.Debug().Err(err).Msg(...) with descriptive messages (e.g., "failed to
register g_type_check_instance_is_a" and "failed to register
g_type_name_from_instance") to match the existing pattern in this file.
Comment on lines
+3
to
+7
| import ( | ||
| "context" | ||
|
|
||
| "github.com/bnema/dumber/internal/domain/entity" | ||
| ) |
There was a problem hiding this comment.
Domain-layer import violates repository guidelines.
context is outside the domain package, which conflicts with the domain isolation rule. Consider moving this interface to the application port layer or introducing a domain-local context abstraction.
As per coding guidelines, Domain layer must not import anything outside the domain package.
🤖 Prompt for AI Agents
In `@internal/domain/repository/permission.go` around lines 3 - 7, The domain
package is importing "context" in internal/domain/repository/permission.go which
breaks domain isolation; either move the repository interface (the permission
repository defined in permission.go) to the application/port layer where
importing context is allowed, or remove the external context dependency by
introducing a domain-local context abstraction (e.g., a DomainContext type or
method signatures without context) and update all usages accordingly; ensure you
reference and update the repository interface in permission.go and any methods
that currently accept context so callers are adjusted to the chosen approach.
Comment on lines
+64
to
+65
| core.PuregoSafeRegister(&typeCheckInstanceIsA, libs, "g_type_check_instance_is_a") | ||
| core.PuregoSafeRegister(&typeNameFromInstance, libs, "g_type_name_from_instance") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file mentioned in the review
head -80 internal/infrastructure/webkit/gobject_type_helpers.go | tail -30Repository: bnema/dumber
Length of output: 934
🏁 Script executed:
# Find where core.PuregoSafeRegister is defined
rg -r "PuregoSafeRegister" --type go -B 2 -A 5Repository: bnema/dumber
Length of output: 452
🏁 Script executed:
# Check the full imports and context of the file
head -100 internal/infrastructure/webkit/gobject_type_helpers.goRepository: bnema/dumber
Length of output: 1782
🏁 Script executed:
# Search for PuregoSafeRegister definition in the codebase
fd -t f "*.go" | xargs grep -l "PuregoSafeRegister" | head -20Repository: bnema/dumber
Length of output: 339
🏁 Script executed:
# Look for where PuregoSafeRegister might be defined
rg "func.*PuregoSafeRegister" --type go -A 10Repository: bnema/dumber
Length of output: 38
🌐 Web query:
PuregoSafeRegister github.com/jwijenbergh/puregotk return error
💡 Result:
PuregoSafeRegister in github.com/jwijenbergh/puregotk/internal/core is a “safe” wrapper used by the generated bindings to register a C symbol into a Go function pointer, but returning an error instead of crashing when the symbol can’t be resolved. (pkg.go.dev)
In practice, the returned error almost always means Dlsym(handle, name) failed (i.e., the symbol name isn’t present in the shared library you loaded, or you loaded the wrong library / wrong version), because purego.RegisterLibFunc is effectively Dlsym + RegisterFunc. (pkg.go.dev)
Common fixes in puregotk setups:
- Ensure you’re loading the right GTK (and dependency)
.so/.dylibfiles for the API version your generated bindings expect. - Set explicit library paths so
puregotkdoesn’t pick up an unexpected system copy (common on NixOS/custom prefixes):PUREGOTK_LIB_FOLDER=/path/to/libs- or per-lib overrides like
PUREGOTK_GTK_PATH=/path/to/libgtk-4.so(and similarly for cairo/pango/etc.). (pkg.go.dev)
- Debug the exact missing symbol by calling
purego.Dlsym(handle, "the_symbol_name")yourself and printing the error to confirm whether it’s a path/version mismatch. (pkg.go.dev)
If you paste the exact error text you get back from PuregoSafeRegister (and which symbol name it was trying to register), I can tell you which library/package/version typically provides that symbol.
Citations:
- 1: https://pkg.go.dev/github.com/jwijenbergh/puregotk/internal/core?utm_source=openai
- 2: https://pkg.go.dev/github.com/ebitengine/purego?utm_source=openai
- 3: https://pkg.go.dev/github.com/jwijenbergh/puregotk?utm_source=openai
- 4: https://pkg.go.dev/github.com/ebitengine/purego?utm_source=openai
🌐 Web query:
site:pkg.go.dev PuregoSafeRegister signature func
💡 Result:
PuregoSafeRegister is declared as:
func PuregoSafeRegister(fptr interface{}, handle uintptr, name string) error[1]
Log errors from PuregoSafeRegister calls.
Both calls to core.PuregoSafeRegister ignore the returned error. When symbol binding fails (e.g., symbol not found in the library), the function pointers remain nil silently without any logging. Add error logging to match the existing pattern in this function and improve debuggability:
if err := core.PuregoSafeRegister(&typeCheckInstanceIsA, libs, "g_type_check_instance_is_a"); err != nil {
log.Debug().Err(err).Msg("failed to register g_type_check_instance_is_a")
}
if err := core.PuregoSafeRegister(&typeNameFromInstance, libs, "g_type_name_from_instance"); err != nil {
log.Debug().Err(err).Msg("failed to register g_type_name_from_instance")
}🤖 Prompt for AI Agents
In `@internal/infrastructure/webkit/gobject_type_helpers.go` around lines 64 - 65,
The calls to core.PuregoSafeRegister currently ignore returned errors, leaving
function pointers like typeCheckInstanceIsA and typeNameFromInstance nil without
logging; update both calls to capture the error (if err :=
core.PuregoSafeRegister(&typeCheckInstanceIsA, libs,
"g_type_check_instance_is_a"); err != nil { ... }) and (if err :=
core.PuregoSafeRegister(&typeNameFromInstance, libs,
"g_type_name_from_instance"); err != nil { ... }) and log failures using
log.Debug().Err(err).Msg(...) with descriptive messages (e.g., "failed to
register g_type_check_instance_is_a" and "failed to register
g_type_name_from_instance") to match the existing pattern in this file.
QueryPermissionState now checks stored decisions before auto-allow defaults so manual denies are respected, and manual decision writes reject non-persistable permission types. Also moves permission repository contract to application port, updates docs for removed revoke API, and hardens IPv6 origin plus GObject helper safety/logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/reference/permissions.md`:
- Around line 36-42: Update the "Origin-Based Storage" section to state that
stored origins include scheme, host, and port (with default ports 80 for http
and 443 for https omitted), e.g., clarify that https://example.com and
https://example.com:8443 are different origins while https://example.com and
https://example.com:443 are the same; edit the paragraph under the "Origin-Based
Storage" heading and adjust the examples (`https://meet.google.com/room1` etc.)
to show how explicit ports affect origin equality.
In `@internal/application/usecase/handle_permission.go`:
- Around line 4-13: The usecase currently imports the adapter package
"internal/logging" (tight coupling); remove that import and instead accept a
logger boundary via the existing port package (e.g., add a port.Logger or
logging function parameter) on the usecase struct and its constructor (e.g.,
New... or NewHandlePermissionUsecase), store it as a field and replace direct
calls to internal/logging with calls to that injected logger; keep all wiring of
internal/logging in the composition root so the usecase only depends on
port.Logger and not the adapter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/application/usecase/handle_permission.go`:
- Around line 141-173: Add an early empty-origin guard in QueryPermissionState:
at the start of the function check if origin == "" and immediately return
entity.PermissionPrompt (matching HandlePermissionRequest behavior) before
calling uc.permRepo.Get or evaluating entity.IsAutoAllow; this prevents
auto-allow types from being reported granted for empty origins. Use the existing
symbols QueryPermissionState, entity.PermissionPrompt, uc.permRepo.Get, and
entity.IsAutoAllow to locate where to add the guard.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/dumber/main.go (1)
509-579:⚠️ Potential issue | 🔴 CriticalFix duplicate fields in buildUIDependencies (currently fails to compile).
Line 545+ repeats the struct literal fields, which is already failing CI with duplicate field errors. Merge into a single struct literal and keep both StartupCrashReports and the new Permission fields.🛠️ Proposed fix
return &ui.Dependencies{ Ctx: ctx, Config: cfg, InitialURL: initialURL, RestoreSessionID: restoreSessionID, + StartupCrashReports: startupCrashReports, Theme: themeManager, ColorResolver: colorResolver, AdwaitaDetector: adwaitaDetector, XDG: xdg.New(), WebContext: stack.Context, Pool: stack.Pool, Settings: stack.Settings, Injector: stack.Injector, MessageRouter: stack.MessageRouter, FilterManager: stack.FilterManager, HistoryRepo: repos.history, FavoriteRepo: repos.favorite, ZoomRepo: repos.zoom, PermissionRepo: repos.permission, TabsUC: uc.tabs, PanesUC: uc.panes, HistoryUC: uc.history, FavoritesUC: uc.favorites, ZoomUC: uc.zoom, PermissionUC: uc.permission, NavigateUC: uc.navigate, CopyURLUC: uc.copyURL, Clipboard: uc.clipboard, FaviconService: uc.favicon, IdleInhibitor: idleInhibitor, SessionRepo: repos.session, SessionStateRepo: repos.sessionState, CurrentSessionID: currentSessionID, SnapshotUC: uc.snapshot, CheckUpdateUC: uc.checkUpdate, ApplyUpdateUC: uc.applyUpdate, - Ctx: ctx, - Config: cfg, - InitialURL: initialURL, - RestoreSessionID: restoreSessionID, - StartupCrashReports: startupCrashReports, - Theme: themeManager, - ColorResolver: colorResolver, - AdwaitaDetector: adwaitaDetector, - XDG: xdg.New(), - WebContext: stack.Context, - Pool: stack.Pool, - Settings: stack.Settings, - Injector: stack.Injector, - MessageRouter: stack.MessageRouter, - FilterManager: stack.FilterManager, - HistoryRepo: repos.history, - FavoriteRepo: repos.favorite, - ZoomRepo: repos.zoom, - TabsUC: uc.tabs, - PanesUC: uc.panes, - HistoryUC: uc.history, - FavoritesUC: uc.favorites, - ZoomUC: uc.zoom, - NavigateUC: uc.navigate, - CopyURLUC: uc.copyURL, - Clipboard: uc.clipboard, - FaviconService: uc.favicon, - IdleInhibitor: idleInhibitor, - SessionRepo: repos.session, - SessionStateRepo: repos.sessionState, - CurrentSessionID: currentSessionID, - SnapshotUC: uc.snapshot, - CheckUpdateUC: uc.checkUpdate, - ApplyUpdateUC: uc.applyUpdate, }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webui/src/pages/webrtc.ts`:
- Around line 31-43: The probeDevices function calls
navigator.mediaDevices.enumerateDevices() without handling rejections; wrap the
await in a try-catch inside probeDevices so any thrown error is caught and
logged via appendLog (include the error message and context like
"enumerateDevices failed"). Keep the existing early check for
navigator.mediaDevices and ensure the catch still appends a useful "Devices:
none" or error-specific message to the same output element used by appendLog so
diagnostics remain clear.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webui/src/pages/webrtc.ts`:
- Around line 56-68: When you obtain a MediaStream in the try block (the code
that calls navigator.mediaDevices.getUserMedia and pushes to activeStreams, sets
video.srcObject and calls video.play), attach listeners to each
stream.getTracks() for the standard "ended" event (and optionally "mute" if
needed); on the handler check stream.active and if false remove that stream from
activeStreams, update setStatus(status, "camera + microphone stopped") and
appendLog(output, "camera + microphone stopped by user"), and perform any
cleanup (e.g., stop tracks, clear video.srcObject, and re-run or abort
probeDevices as appropriate). Apply the same change to the other similar block
around lines 77–88 that also creates/uses activeStreams so both places use track
"ended" handling and stream.active checks to keep status/logs in sync.
Comment on lines
+56
to
+68
| try { | ||
| setStatus(status, "requesting camera + microphone"); | ||
| const stream = await navigator.mediaDevices.getUserMedia({ audio: true, video: true }); | ||
| activeStreams.push(stream); | ||
| video.srcObject = stream; | ||
| await video.play(); | ||
| setStatus(status, "camera + microphone granted"); | ||
| appendLog(output, "camera + microphone permission granted"); | ||
| await probeDevices(output); | ||
| } catch (err) { | ||
| setStatus(status, "camera + microphone blocked"); | ||
| appendLog(output, `camera + microphone request failed: ${String(err)}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MediaStream inactive event support WebKitGTK browser compatibility
💡 Result:
MediaStream’s inactive event (stream.oninactive / addEventListener("inactive", …)) is non‑standard and you should not rely on it for cross‑browser compatibility (including WebKitGTK). MDN explicitly labels MediaStream active/inactive as non-standard and advises checking support before using them. [1]
What to do instead (works broadly, including WebKit/WebKitGTK):
- Prefer
MediaStreamTracklifecycle:- Listen for
track.onended/track.addEventListener("ended", …)to know when capture stops. [2] - Optionally also watch
mute/unmute(temporary loss vs permanent end).
- Listen for
- For stream-level state, poll/check
stream.active(it becomesfalseonce all tracks’readyStateareended). [3] - If you need to detect track set changes, use
addtrack/removetrackon theMediaStream. [1]
Why this matters for WebKitGTK compatibility:
- Historically, engines disagreed: Blink even planned removals of older stream-level events and noted interoperability issues; in that context, WebKit was cited as not supporting the old
MediaStreamendedevent whileactive/inactivewere treated separately. [4] - The
active/inactivestream events aren’t in the standardized event set (and have been discussed as undocumented/inconsistent across browsers). [5]
Bottom line: treat MediaStream inactive as unreliable/non-portable; implement stream “ended” logic by observing tracks (ended, readyState) plus stream.active. [1][2][3]
Sources: [1] MDN MediaStream (events) • [2] MDN MediaStreamTrack ended event • [3] MDN MediaStream active property • [4] blink-dev deprecation discussion • [5] discuss-webrtc thread on active/inactive non-standard status.
🌐 Web query:
MediaStream inactive event fires when all tracks end JavaScript
💡 Result:
MediaStream’s inactive event exists in some browsers, but it’s non‑standard, so you shouldn’t rely on it for “all tracks ended” detection. [4]
The standard way is:
- A
MediaStreamis considered inactive when all its tracks areended; at that pointstream.activebecomesfalse. [1] - Listen for each
MediaStreamTrack’sendedevent, then check whether the whole stream is now inactive. [2] - Note: if you call
track.stop(), the track’s state becomesendedbut theendedevent will not fire, so you must handle that path yourself. [3]
function onStreamInactive(stream, cb) {
const check = () => { if (!stream.active) cb(); }; // false when all tracks ended [1]
const wireTrack = (t) => t.addEventListener("ended", check); // fires when track naturally ends [2]
stream.getTracks().forEach(wireTrack);
// if tracks can be added later
stream.addEventListener("addtrack", (e) => wireTrack(e.track));
// if you stop tracks yourself, call check after stop() because 'ended' won't fire [3]
return { check };
}
// usage
const { check } = onStreamInactive(stream, () => console.log("all tracks ended"));
stream.getTracks().forEach(t => t.stop());
check(); // required if you stop() them yourselfSources: MDN on MediaStream.active semantics [1], MediaStreamTrack ended event [2], MediaStreamTrack.stop() not firing ended [3], and MediaStream inactive being non-standard [4].
🏁 Script executed:
# Check the actual webrtc.ts file to understand the full context
find . -name "webrtc.ts" -type fRepository: bnema/dumber
Length of output: 83
🏁 Script executed:
# Read the webrtc.ts file to understand the full context
cat -n ./webui/src/pages/webrtc.ts | head -120Repository: bnema/dumber
Length of output: 5499
🏁 Script executed:
# Continue reading the rest of webrtc.ts
cat -n ./webui/src/pages/webrtc.ts | tail -50Repository: bnema/dumber
Length of output: 1735
Use standard MediaStreamTrack ended events instead of non-standard inactive to detect when the user stops capture.
If the user stops sharing from the browser UI, the stream's tracks end but activeStreams retains the stream and status/logs stay "granted," making diagnostics misleading. Listen for each track's ended event and check stream.active to detect this condition and clean up.
🛠️ Suggested fix
try {
setStatus(status, "requesting camera + microphone");
const stream = await navigator.mediaDevices.getUserMedia({ audio: true, video: true });
activeStreams.push(stream);
+ const onStreamInactive = () => {
+ const idx = activeStreams.indexOf(stream);
+ if (idx >= 0) {
+ activeStreams.splice(idx, 1);
+ }
+ if (video.srcObject === stream) {
+ video.pause();
+ video.srcObject = null;
+ }
+ setStatus(status, "idle");
+ appendLog(output, "camera + microphone stream ended");
+ };
+ stream.getTracks().forEach((track) => {
+ track.addEventListener("ended", onStreamInactive);
+ });
video.srcObject = stream;
await video.play();
setStatus(status, "camera + microphone granted");
appendLog(output, "camera + microphone permission granted");
await probeDevices(output);
} catch (err) {
@@
try {
setStatus(status, "requesting screen capture");
const stream = await navigator.mediaDevices.getDisplayMedia({ video: true, audio: true });
activeStreams.push(stream);
+ const onStreamInactive = () => {
+ const idx = activeStreams.indexOf(stream);
+ if (idx >= 0) {
+ activeStreams.splice(idx, 1);
+ }
+ if (video.srcObject === stream) {
+ video.pause();
+ video.srcObject = null;
+ }
+ setStatus(status, "idle");
+ appendLog(output, "screen capture stream ended");
+ };
+ stream.getTracks().forEach((track) => {
+ track.addEventListener("ended", onStreamInactive);
+ });
video.srcObject = stream;
await video.play();
setStatus(status, "screen capture granted");
appendLog(output, "screen capture permission granted");
} catch (err) {Also applies to: 77–88
🤖 Prompt for AI Agents
In `@webui/src/pages/webrtc.ts` around lines 56 - 68, When you obtain a
MediaStream in the try block (the code that calls
navigator.mediaDevices.getUserMedia and pushes to activeStreams, sets
video.srcObject and calls video.play), attach listeners to each
stream.getTracks() for the standard "ended" event (and optionally "mute" if
needed); on the handler check stream.active and if false remove that stream from
activeStreams, update setStatus(status, "camera + microphone stopped") and
appendLog(output, "camera + microphone stopped by user"), and perform any
cleanup (e.g., stop tracks, clear video.srcObject, and re-run or abort
probeDevices as appropriate). Apply the same change to the other similar block
around lines 77–88 that also creates/uses activeStreams so both places use track
"ended" handling and stream.active checks to keep status/logs in sync.
bnema
added a commit
that referenced
this pull request
Feb 9, 2026
feat: implement media permission system
This was referenced Mar 15, 2026
4 tasks
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
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
This PR implements a complete media permission system for the dumber browser, handling WebKit permission requests for microphone, camera, display capture, and device enumeration.
Changes
Domain Layer
PermissionType,PermissionDecision,PermissionRecordentitiesCanPersist,IsAutoAllow)PermissionRepositoryinterface for storage abstractionInfrastructure Layer
PermissionRepositorywith CRUD operationsApplication Layer
HandlePermissionUseCasewith auto-allow and dialog flowPermissionDialogPresenterinterface for UI abstractionUI Layer
Wiring
Key Behaviors
Testing
All tests pass:
make lint- 0 issuesmake test- All permission tests passCommits
501c1f1- Domain entities and repository interfaces5a0e167- SQLite repository implementation8ed37d6- Use case and dialog presenter interfacesa461e2a- WebKit permission request handling9d0f4b3- UI dialog implementation483f968- Wire permission system into maina72fd77- Add mocks and update configurationSummary by CodeRabbit
New Features
Documentation
Known upstream limitation
Screen sharing (
getDisplayMedia) is currently blocked by a WebKitGTK/GStreamer/PipeWire caps-negotiation issue on some Wayland setups. In these cases, permission is granted and capture state becomes active, but no video frames are delivered to the page preview. The failure reproduces in Dumber and Epiphany on the same machine, which indicates an upstream runtime issue rather than Dumber permission logic.This PR keeps the permission pipeline changes, and we will track runtime mitigation separately (forked runtime and/or upstream patch flow).