Repository navigation
UI upgrades notifications and manual check - #3
Conversation
- Added periodic upgrade checking mechanism in the App struct, including methods for checking updates and dismissing notifications. - Integrated version management with the new version package, allowing the application to fetch and compare versions from GitHub. - Updated application configuration to include fields for upgrade check settings, such as auto-check and last upgrade check timestamp. - Enhanced frontend with UpgradeNotification component and settings for managing upgrade preferences. - Updated Go modules to include necessary dependencies for version handling. This commit lays the groundwork for notifying users about available updates and managing upgrade settings effectively.
📝 WalkthroughWalkthroughImplements an automatic upgrade-check feature spanning backend (Go) and frontend (TypeScript/React): new version package, GitHub release fetching, semantic version comparison, periodic/background checks, event-driven notifications, Wails bindings, UI components for notifications and settings, build-time version injection, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Go App
participant Scheduler as Scheduler/Ticker
participant Version as Version pkg
participant GitHub as GitHub API
participant Config as Config Manager
participant Event as Runtime/Event
participant Frontend as Frontend
App->>Scheduler: StartPeriodicUpgradeCheck()
Scheduler->>App: Tick -> trigger check
App->>Version: CheckForUpdates()
Version->>Version: GetVersion() / normalize
Version->>GitHub: FetchLatestRelease()
GitHub-->>Version: GitHubRelease JSON
Version->>Version: parse & compare semver
Version-->>App: UpdateInfo
App->>Config: Read DismissedUpgradeVersion
alt update available and not dismissed
App->>Config: Persist LastUpgradeCheck
App->>Event: Emit "upgrade:available" (payload UpdateInfo)
Event->>Frontend: Notify
Frontend->>Frontend: Show UpgradeNotification
else no update or dismissed
App->>App: No event
end
sequenceDiagram
participant User as User
participant UI as Frontend UI
participant Wails as Wails Bridge
participant App as Go App
participant Browser as Browser Runtime
User->>UI: Open Settings -> Upgrade tab
UI->>Wails: GetCurrentVersion()
Wails->>App: GetCurrentVersion()
App-->>Wails: "1.0.0"
Wails-->>UI: "1.0.0"
User->>UI: Click "Check for Updates"
UI->>Wails: CheckForUpdates()
Wails->>App: CheckForUpdates()
App-->>Wails: UpdateInfo
Wails-->>UI: UpdateInfo
alt update available
UI->>User: Show "Download" and "Dismiss"
User->>UI: Click "Download"
UI->>Wails: OpenReleasesPage(url)
Wails->>App: OpenReleasesPage(url)
App->>Browser: Open URL
else dismiss
User->>UI: Click "Dismiss"
UI->>Wails: DismissUpgradeNotification(version)
Wails->>App: DismissUpgradeNotification(version)
App->>Config: Save dismissed version
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI Agents
In @.cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.md:
- Around line 1299-1351: CreateFromTemplate dereferences req.Overrides without
nil checks causing panics; guard uses of req.Overrides (e.g., reading
DisableDeadLetter and passing overrides into createTopic/createSubscription) by
first assigning a local var like overrides := req.Overrides and checking if
overrides != nil before accessing fields (e.g., if overrides != nil &&
overrides.DisableDeadLetter) or pass nil through to helper methods; update the
dead-letter creation branch and the subscription creation loop to use these
nil-safe checks so no field access occurs on a nil Overrides pointer while still
passing overrides to createTopic/createSubscription unchanged.
In @app.go:
- Around line 235-268: The scheduled upgrade-check goroutine can leak because
the ticker loop in scheduleNextUpgradeCheck (for range a.upgradeCheckTicker.C)
never exits and the time.AfterFunc timer isn’t stored so pending callbacks can’t
be cancelled; to fix, add fields to App (e.g., upgradeCheckDone chan struct{}
and upgradeCheckTimer *time.Timer), update scheduleNextUpgradeCheck to create a
fresh done channel under a.upgradeCheckMu, store the result of time.AfterFunc in
a.upgradeCheckTimer (so you can Stop() it if restarting), and run the periodic
goroutine with a select that exits when a.upgradeCheckDone is closed or when
ticker.C fires; update Disconnect (and any restart path) to stop and nil the
ticker, Stop() the timer if set, and close the done channel safely (only once)
while holding upgradeCheckMu so the goroutine can exit cleanly and no duplicate
callbacks remain.
In @frontend/src/components/Settings/UpgradeTab.tsx:
- Around line 88-95: The async handler handleAutoCheckChange lacks error
handling for GetConfigFileContent and SaveConfigFileContent; wrap the async
operations in a try/catch, keep calling setAutoCheckUpgrades immediately, and on
failure revert state or show feedback (e.g., call an existing notify/error
handler) and log the error; specifically update handleAutoCheckChange to catch
exceptions from GetConfigFileContent and SaveConfigFileContent, call the
appropriate UI error notification, and ensure state consistency if saving fails.
- Around line 97-104: handleIntervalChange performs multiple async steps
(GetConfigFileContent, JSON.parse, SaveConfigFileContent) without error
handling; wrap the body of handleIntervalChange in a try/catch, keep calling
setUpgradeCheckInterval on success, and in the catch log or surface the error
(e.g., use your existing logger or show a user-facing error) and optionally
revert state if needed; ensure errors from GetConfigFileContent, JSON.parse, and
SaveConfigFileContent are caught and handled to avoid unhandled promise
rejections.
- Line 112: The code is masking a type mismatch by using setCheckResult(result
as any); instead of properly typing the value returned by CheckForUpdates(); fix
by aligning the state type with the Wails-generated type: either change the
checkResult state to use useState<version.UpdateInfo | null>(null) and keep
using the Wails result, or import the Wails-generated UpdateInfo type/class and
use that for the checkResult state and any related local typings; update any
references to checkResult and setCheckResult accordingly so no "as any"
assertions remain and the types match CheckForUpdates()'s
Promise<version.UpdateInfo>.
In @frontend/src/components/UpgradeNotification.tsx:
- Around line 3-11: The import list in UpgradeNotification.tsx includes
GetCurrentVersion which is unused; remove GetCurrentVersion from the named
import list that currently imports GetCurrentVersion, CheckForUpdates,
DismissUpgradeNotification, OpenReleasesPage from '../wailsjs/go/main/App' so
only the used functions remain (e.g., CheckForUpdates,
DismissUpgradeNotification, OpenReleasesPage), update the import statement
accordingly to eliminate the unused symbol and clean up lint warnings.
- Around line 13-14: The component UpgradeNotification destructures theme from
useTheme() but never uses it; remove the unused variable by deleting "const {
theme } = useTheme();" and, if useTheme is no longer used elsewhere in the file,
also remove the corresponding import (useTheme) to avoid dead code and lint
warnings.
In @internal/version/checker_test.go:
- Around line 93-96: The test "dev with suffix" uses input "123-dev" but
CheckForUpdates() in internal/version/checker.go detects dev builds only via
currentVersion == "dev" || strings.HasPrefix(strings.ToLower(currentVersion),
"dev"), so the test is invalid; either remove or update the test case to use a
matching dev form (e.g., "dev" or "dev-..."), or change the detection logic in
CheckForUpdates() to also treat versions with a trailing "-dev" (e.g., use
strings.Contains(strings.ToLower(currentVersion), "dev") or check for suffix
"-dev") so "123-dev" is recognized as a dev build; update tests accordingly
(adjust the case named "dev with suffix" or the CheckForUpdates() dev-detection
branch).
🧹 Nitpick comments (17)
.cursor/rules/wails.mdc (1)
262-315: Document mutex strategy: clarify whether config access uses dedicated or shared mutex.The new CRITICAL section on config field access in goroutines is helpful, but introduces
upgradeCheckMuas a dedicated mutex for upgrade-check config fields. However, the existing codebase pattern uses shared mutexes (resourceMufor resources, etc.).Consider documenting whether:
- Option A (preferred for consistency): All config field access uses a single
configMumutex- Option B (current pattern): Different subsystems get dedicated mutexes (upgradeCheckMu, monitorsMu, etc.)
If Option B is intentional, add a note explaining the rationale.
🔎 Consider consolidating to single configMu for consistency
- a.upgradeCheckMu.Lock() - dismissedVersion := "" - if a.config != nil { - dismissedVersion = a.config.DismissedUpgradeVersion - } - a.upgradeCheckMu.Unlock() + a.configMu.Lock() + dismissedVersion := "" + if a.config != nil { + dismissedVersion = a.config.DismissedUpgradeVersion + } + a.configMu.Unlock()This aligns with the stated pattern in the wails.mdc: "Use
sync.RWMutexfor read-heavy shared state.".cursor/plans/upgrade_check_feature_implementation_7e439e70.plan.md (2)
1-168: Clarify task status semantics in planning document.The YAML frontmatter marks tasks as "completed" but this is a planning document. Consider using statuses like "planned" or "ready-to-implement" to distinguish planning completeness from actual implementation completion.
This is a documentation improvement that would reduce confusion when referencing the plan later.
204-437: Fix markdown list indentation for consistency.Static analysis flags MD007 (unordered list indentation) and MD005 (inconsistent indentation) throughout the document. The lists should use consistent 2-space indentation per markdownlint rules.
While this doesn't affect functionality, fixing indentation improves document consistency:
- **File**: `internal/version/version.go` + - **File**: `internal/version/version.go` - - Define version constants (GitHubOwner: "B87", GitHubRepo: "pubsub-gui") + - Define version constants (GitHubOwner: "B87", GitHubRepo: "pubsub-gui")Use
mdformator similar tool to auto-fix list indentation..cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.md (2)
442-456: Data models are comprehensive but one field name needs clarification.The template data models cover all major Pub/Sub settings appropriately. However,
DeadLetterTemplateConfig.MessageRetentionDuration2(line 454) is confusing.Suggest renaming for clarity:
- MessageRetentionDuration2 string `json:"messageRetentionDuration2"` + DeadLetterSubMessageRetention string `json:"deadLetterSubMessageRetention"`This makes it clear this is the message retention for the dead-letter subscription, not the topic.
141-141: Add language specifiers to fenced code blocks for markdown consistency.Static analysis flags MD040 (missing language specifier) on several code blocks. While this doesn't affect functionality, it improves readability and consistency:
- ``` + ```plaintext delay = min(min_backoff * 2^attempt, max_backoff) - ``` + ``` - ``` + ```plaintext attributes.region = "us-west1" - ``` + ```This is purely cosmetic but helps syntax highlighters and markdown processors handle the content correctly.
Also applies to: 187-187, 224-224, 310-310, 341-341, 1820-1820
internal/version/github_test.go (3)
79-99: Empty test body does not validate actual behavior.
TestFetchLatestRelease_SkipDraftandTestFetchLatestRelease_SkipPrerelease(lines 79-121) contain emptyifblocks that don't assert anything meaningful. These tests only verify that the struct fields are set correctly, not thatFetchLatestReleaseactually skips drafts/prereleases. This is already covered more comprehensively byTestFetchLatestRelease_DraftPrereleaseSkipping.Consider removing these redundant tests or refactoring to actually call
FetchLatestReleasewith a mock server returning draft/prerelease releases to verify the error is returned.
307-361: Test does not verify actualFetchLatestReleaseUser-Agent behavior.This test manually sets the User-Agent header (line 350) and verifies the server receives it, but it doesn't actually call
FetchLatestRelease(). The test simulates what the function does rather than testing the function itself.Additionally, line 311's
defer SetVersion("dev")won't reset the version to empty becauseSetVersionignores empty strings but accepts "dev" as a valid value - this is fine but the comment implies it's restoring a default state.Since
FetchLatestReleaseuses a hardcoded URL fromGetReleasesURL(), consider refactoring to accept an optional URL parameter or HTTP client for better testability.
205-248: Error handling test doesn't actually testFetchLatestRelease.The test simulates error scenarios that
FetchLatestReleasewould encounter but doesn't call the actual function. The comment on lines 213-215 acknowledges this limitation.Also, line 209 ignores the return value of
w.Write().🔎 Minor fix for unchecked write
- w.Write([]byte(tt.responseBody)) + _, _ = w.Write([]byte(tt.responseBody))internal/version/github.go (1)
49-52: Consider using a sentinel error for draft/prerelease detection.The current error message is descriptive but upstream callers cannot easily distinguish this "expected" scenario (no stable release available) from actual failures. A sentinel error would allow callers to handle this case differently.
🔎 Suggested improvement
// At package level: var ErrNoStableRelease = errors.New("latest release is draft or prerelease") // In function: if release.Draft || release.Prerelease { return nil, ErrNoStableRelease }frontend/src/components/UpgradeNotification.tsx (1)
24-32: Extract duplicate UpdateInfo conversion logic.The conversion from
version.UpdateInfotoUpdateInfois duplicated in both the event handler (lines 24-32) andhandleCheckNow(lines 80-88).🔎 Proposed refactor
// Add helper function at the top of the component or as a utility const mapToUpdateInfo = (data: version.UpdateInfo): UpdateInfo => ({ currentVersion: data.currentVersion, latestVersion: data.latestVersion, releaseNotes: data.releaseNotes, releaseUrl: data.releaseUrl, publishedAt: data.publishedAt, isUpdateAvailable: data.isUpdateAvailable, }); // Then use in both places: setUpdateInfo(mapToUpdateInfo(data));Also applies to: 80-88
frontend/src/components/SettingsDialog.tsx (1)
499-512: Consider adding footer handling for the 'upgrade' tab.The footer conditionally shows buttons for 'appearance', 'connections', and 'advanced' tabs, but the new 'upgrade' tab is not included. This means no footer buttons appear when on the upgrade tab.
Looking at the UpgradeTab implementation (from relevant snippets), it handles its own "Check for Updates Now" button internally. If this is intentional, consider adding a simple "Close" button in the footer for consistency, or explicitly document that the upgrade tab manages its own actions.
🔎 Suggested addition
- {(activeTab === 'appearance' || activeTab === 'connections') && ( + {(activeTab === 'appearance' || activeTab === 'connections' || activeTab === 'upgrade') && (frontend/src/types/upgrade.ts (1)
1-17: Consider exporting fromtypes/index.tsper coding guidelines.Per coding guidelines: "Define all TypeScript types and interfaces in
types/index.ts. Export them for use across components."The types are correctly defined and align with the backend structs, but they should ideally be exported from
types/index.tsfor consistency with the project structure.Additionally, note that
frontend/wailsjs/go/models.ts(lines 253-274 in relevant snippets) already generates anUpdateInfoclass from the Go struct. Consider whether you need this separate interface or can use the Wails-generated type directly.app_test.go (3)
51-52: Inconsistent config initialization pattern.This test initializes
app.configwith an emptyAppConfig{}, whileTestCheckForUpdates_UpdatesLastUpgradeCheck(line 83) usesmodels.NewDefaultConfig(). The empty initialization might miss required fields with non-zero defaults defined inNewDefaultConfig().🔎 Proposed fix
- // Initialize minimal config for test - app.config = &models.AppConfig{} + // Initialize config with defaults + app.config = models.NewDefaultConfig()
91-93: Confusing double-negative logic.The condition
!initialTime.IsZero() && !initialTime.Before(time.Now())is logically correct but hard to parse due to double negatives. Consider a more readable assertion.🔎 Clearer alternative
- if !initialTime.IsZero() && !initialTime.Before(time.Now()) { - t.Error("initial LastUpgradeCheck should be zero or in the past") + if !initialTime.IsZero() && initialTime.After(time.Now()) { + t.Error("initial LastUpgradeCheck should be zero or in the past") }
104-121: Unclear test validation logic.The test comment states "the actual version check will fail" (line 113), but the assertion logic (lines 117-120) accepts both error and success cases. This makes it unclear what behavior is actually being validated beyond "doesn't panic."
Consider clarifying the test's purpose: Is it testing graceful nil
configManagerhandling, or dev-build behavior, or network failure resilience?frontend/src/components/Settings/UpgradeTab.tsx (1)
69-86: Dead code:saveSettingsfunction is never called.The
saveSettingsfunction is defined but unused. Settings are saved directly inhandleAutoCheckChangeandhandleIntervalChangeinstead. Either remove this function or refactor the handlers to use it.🔎 Proposed refactor
Remove the unused function:
- const saveSettings = async () => { - setError(''); - try { - const content = await GetConfigFileContent(); - const config = JSON.parse(content); - - config.autoCheckUpgrades = autoCheckUpgrades; - config.upgradeCheckInterval = upgradeCheckInterval; - - const updatedContent = JSON.stringify(config, null, 2); - await SaveConfigFileContent(updatedContent); - - // Reload to get updated last check time - await loadSettings(); - } catch (e: any) { - setError('Failed to save settings: ' + e.toString()); - } - }; -Or refactor handlers to use it (after adding error handling to those handlers).
app.go (1)
320-328: Consider adding URL scheme validation.The URL is expected to come from GitHub API responses, which should be trustworthy. However, as defense-in-depth, consider validating that the URL uses an expected scheme (
https://).🔎 Optional: Add URL scheme validation
+import "strings" func (a *App) OpenReleasesPage(url string) error { if url == "" { return fmt.Errorf("URL cannot be empty") } + if !strings.HasPrefix(url, "https://") { + return fmt.Errorf("invalid URL scheme: only HTTPS URLs are allowed") + } runtime.BrowserOpenURL(a.ctx, url) return nil }
| setCheckResult(null); | ||
| try { | ||
| const result = await CheckForUpdates(); | ||
| setCheckResult(result as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check UpdateInfo type definition in bindings vs local types
echo "=== Wails binding type for CheckForUpdates ==="
rg -A 20 "CheckForUpdates" frontend/wailsjs/go/main/App.d.ts | head -40
echo -e "\n=== Local UpdateInfo type ==="
rg -A 10 "interface UpdateInfo" frontend/src/types/Repository: B87/pubsub-gui
Length of output: 1454
🏁 Script executed:
#!/bin/bash
# Find version.UpdateInfo definition in Wails bindings
echo "=== Searching for version.UpdateInfo in Wails bindings ==="
find frontend/wailsjs -name "*.d.ts" -o -name "*.ts" | xargs rg -l "version" | head -5
echo -e "\n=== Looking for UpdateInfo in version namespace ==="
rg "UpdateInfo" frontend/wailsjs/ | head -20
echo -e "\n=== Context around line 112 in UpgradeTab.tsx ==="
sed -n '100,125p' frontend/src/components/Settings/UpgradeTab.tsxRepository: B87/pubsub-gui
Length of output: 1309
🏁 Script executed:
#!/bin/bash
echo "=== version.UpdateInfo class definition ==="
rg -A 30 "export class UpdateInfo" frontend/wailsjs/go/models.tsRepository: B87/pubsub-gui
Length of output: 839
🏁 Script executed:
#!/bin/bash
echo "=== Finding checkResult state declaration ==="
sed -n '1,100p' frontend/src/components/Settings/UpgradeTab.tsx | rg -A 2 "checkResult"Repository: B87/pubsub-gui
Length of output: 268
Remove as any type assertion and properly type the result.
The CheckForUpdates() method returns Promise<version.UpdateInfo> (from Wails bindings), but checkResult state expects UpdateInfo interface. While both have identical field structures, using as any masks a real type mismatch between the generated class and the local interface.
Change to either:
- Update state type to
useState<version.UpdateInfo | null>(null), or - Import and use the Wails-generated
UpdateInfoclass instead of the local interface
This ensures type safety and aligns with the coding guideline to use proper type conversion for Wails objects rather than bypassing TypeScript checks.
🤖 Prompt for AI Agents
In @frontend/src/components/Settings/UpgradeTab.tsx at line 112, The code is
masking a type mismatch by using setCheckResult(result as any); instead of
properly typing the value returned by CheckForUpdates(); fix by aligning the
state type with the Wails-generated type: either change the checkResult state to
use useState<version.UpdateInfo | null>(null) and keep using the Wails result,
or import the Wails-generated UpdateInfo type/class and use that for the
checkResult state and any related local typings; update any references to
checkResult and setCheckResult accordingly so no "as any" assertions remain and
the types match CheckForUpdates()'s Promise<version.UpdateInfo>.
- Introduced a timer and a done channel in the App struct to manage upgrade checks more effectively, allowing for clean exits of goroutines. - Updated the scheduleNextUpgradeCheck method to stop existing timers and handle channel closures safely. - Improved error handling in the UpgradeTab component for auto-check and interval settings, ensuring UI state reverts on failure. - Refactored the UpgradeNotification component by removing unnecessary theme imports. These changes improve the robustness of the upgrade checking process and enhance user experience in managing upgrade settings.
Greetile Summary
Important Files Changed
Confidence score: 2/5
Sequence DiagramsequenceDiagram
participant User
participant App
participant Version
participant GitHub
participant Frontend
participant Browser
User->>App: "Start Application"
App->>App: "startup()"
App->>App: "StartPeriodicUpgradeCheck()"
alt config.AutoCheckUpgrades == true
App->>App: "scheduleNextUpgradeCheck()"
App->>App: "performUpgradeCheck()"
App->>Version: "CheckForUpdates()"
alt current version != "dev"
Version->>GitHub: "FetchLatestRelease()"
GitHub-->>Version: "GitHubRelease"
Version->>Version: "Compare versions"
alt update available
Version-->>App: "UpdateInfo{isUpdateAvailable: true}"
alt not dismissed
App->>Frontend: "EventsEmit('upgrade:available', updateInfo)"
Frontend->>Frontend: "Show UpgradeNotification"
User->>Frontend: "Click Download Update"
Frontend->>App: "OpenReleasesPage(url)"
App->>Browser: "runtime.BrowserOpenURL()"
User->>Frontend: "Click Dismiss"
Frontend->>App: "DismissUpgradeNotification(version)"
App->>App: "Save dismissed version to config"
end
end
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| ../wailsjs No newline at end of file | |||
There was a problem hiding this comment.
logic: This symlink should be removed. Wails auto-generates bindings in frontend/wailsjs/ and components should import directly using ../wailsjs/go/main/App rather than creating symlinks that could interfere with the generation process.
Context Used: Context from dashboard - Wails v2 Application Development Guide (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/wailsjs
Line: 1:1
Comment:
**logic:** This symlink should be removed. Wails auto-generates bindings in `frontend/wailsjs/` and components should import directly using `../wailsjs/go/main/App` rather than creating symlinks that could interfere with the generation process.
**Context Used:** Context from `dashboard` - Wails v2 Application Development Guide ([source](https://app.greptile.com/review/custom-context?memory=2a6d6324-ea6d-4761-9f38-5ee98a08bfd4))
How can I resolve this? If you propose a fix, please make it concise.| Templates []MessageTemplate `json:"templates"` // Message templates | ||
| AutoCheckUpgrades bool `json:"autoCheckUpgrades"` | ||
| UpgradeCheckInterval int `json:"upgradeCheckInterval"` // hours | ||
| LastUpgradeCheck time.Time `json:"lastUpgradeCheck,omitempty"` |
There was a problem hiding this comment.
style: Consider using time.Duration instead of int for UpgradeCheckInterval to make the units explicit and type-safe
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/models/connection.go
Line: 35:35
Comment:
**style:** Consider using `time.Duration` instead of `int` for UpgradeCheckInterval to make the units explicit and type-safe
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| import { useTheme } from '../hooks/useTheme'; | ||
|
|
||
| export default function UpgradeNotification() { | ||
| const { theme } = useTheme(); |
There was a problem hiding this comment.
style: The theme variable is imported from useTheme() but never used in the component logic.
| const { theme } = useTheme(); | |
| const {} = useTheme(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/UpgradeNotification.tsx
Line: 14:14
Comment:
**style:** The `theme` variable is imported from `useTheme()` but never used in the component logic.
```suggestion
const {} = useTheme();
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| setError(''); | ||
| try { | ||
| await OpenReleasesPage(updateInfo.releaseUrl); | ||
| setError(''); |
There was a problem hiding this comment.
style: Redundant error clearing - setError('') is already called on line 65.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/UpgradeNotification.tsx
Line: 68:68
Comment:
**style:** Redundant error clearing - `setError('')` is already called on line 65.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
| setUpdateInfo(updateInfo); | ||
| setIsVisible(true); | ||
| setError(''); |
There was a problem hiding this comment.
style: Redundant error clearing - setError('') is already called on line 76.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/UpgradeNotification.tsx
Line: 91:91
Comment:
**style:** Redundant error clearing - `setError('')` is already called on line 76.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| if mockPrerelease.Draft || mockPrerelease.Prerelease { | ||
| // This is the condition that causes FetchLatestRelease to return an error | ||
| // We're testing the logic, not the actual HTTP call | ||
| } |
There was a problem hiding this comment.
style: Same issue as above - empty conditional block. The comment explains the intent but no actual validation occurs.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/version/github_test.go
Line: 117:120
Comment:
**style:** Same issue as above - empty conditional block. The comment explains the intent but no actual validation occurs.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| Prerelease: false, | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(release) |
There was a problem hiding this comment.
logic: Missing error check for json.NewEncoder(w).Encode(release) - should handle potential encoding errors in test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/version/github_test.go
Line: 335:335
Comment:
**logic:** Missing error check for json.NewEncoder(w).Encode(release) - should handle potential encoding errors in test.
How can I resolve this? If you propose a fix, please make it concise.| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "pubsub-gui/test" | ||
| "pubsub-gui/internal/config" | ||
| "pubsub-gui/internal/models" | ||
| ) | ||
|
|
||
| // setupIntegrationTestApp creates an App instance configured for integration testing with emulator | ||
| func setupIntegrationTestApp(t *testing.T) (*App, func()) { | ||
| t.Helper() | ||
|
|
||
| // Start emulator | ||
| _, cleanupEmulator := test.StartEmulator(t) | ||
|
|
||
| // Setup test config directory | ||
| _, cleanupConfig := test.SetupTestConfigDir(t) | ||
|
|
||
| // Create app | ||
| func TestGetCurrentVersion(t *testing.T) { | ||
| app := NewApp() | ||
| // Use context with longer timeout for integration tests (60 seconds for emulator operations) | ||
| ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) | ||
| t.Cleanup(cancel) | ||
| app.startup(ctx) | ||
|
|
||
| cleanup := func() { | ||
| app.Disconnect() | ||
| cleanupConfig() | ||
| cleanupEmulator() | ||
| tests := []struct { | ||
| name string | ||
| version string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "set version v1.0.0", | ||
| version: "v1.0.0", | ||
| want: "v1.0.0", | ||
| }, | ||
| { | ||
| name: "set version 1.2.3", | ||
| version: "1.2.3", | ||
| want: "1.2.3", | ||
| }, | ||
| { | ||
| name: "empty version returns dev", | ||
| version: "", | ||
| want: "dev", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| app.SetVersion(tt.version) | ||
| got := app.GetCurrentVersion() | ||
| if got != tt.want { | ||
| t.Errorf("GetCurrentVersion() = %q, want %q", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| return app, cleanup | ||
| } | ||
|
|
||
| // TestConnectWithADC_Integration tests connecting to emulator using ADC | ||
| func TestConnectWithADC_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
| func TestCheckForUpdates_DevBuild(t *testing.T) { | ||
| app := NewApp() | ||
| app.SetVersion("dev") | ||
|
|
||
| projectID := "test-project" | ||
| // Initialize minimal config for test | ||
| app.config = &models.AppConfig{} | ||
|
|
||
| err := app.ConnectWithADC(projectID) | ||
| got, err := app.CheckForUpdates() | ||
| if err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| t.Errorf("CheckForUpdates() error = %v, want nil", err) | ||
| return | ||
| } | ||
|
|
||
| // Verify connection status | ||
| status := app.GetConnectionStatus() | ||
| if !status.IsConnected { | ||
| t.Error("ConnectWithADC() IsConnected = false, want true") | ||
| } | ||
| if status.ProjectID != projectID { | ||
| t.Errorf("ConnectWithADC() ProjectID = %q, want %q", status.ProjectID, projectID) | ||
| if got == nil { | ||
| t.Fatal("CheckForUpdates() = nil, want UpdateInfo") | ||
| } | ||
| if status.EmulatorHost == "" { | ||
| t.Error("ConnectWithADC() EmulatorHost is empty, should be set when using emulator") | ||
| } | ||
| } | ||
|
|
||
| // TestPublishMessage_Integration tests publishing messages using emulator | ||
| func TestPublishMessage_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| if got.CurrentVersion != "dev" { | ||
| t.Errorf("CheckForUpdates() CurrentVersion = %q, want %q", got.CurrentVersion, "dev") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "test-topic" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Publish message | ||
| payload := `{"test": "data"}` | ||
| attributes := map[string]string{ | ||
| "key1": "value1", | ||
| "key2": "value2", | ||
| } | ||
|
|
||
| got, err := app.PublishMessage(topicID, payload, attributes) | ||
| if err != nil { | ||
| t.Fatalf("PublishMessage() error = %v", err) | ||
| } | ||
|
|
||
| if got.MessageID == "" { | ||
| t.Error("PublishMessage() MessageID is empty") | ||
| } | ||
| if got.Timestamp == "" { | ||
| t.Error("PublishMessage() Timestamp is empty") | ||
| } | ||
|
|
||
| // Verify timestamp is valid RFC3339 | ||
| _, err = time.Parse(time.RFC3339, got.Timestamp) | ||
| if err != nil { | ||
| t.Errorf("PublishMessage() Timestamp = %q, want valid RFC3339 format", got.Timestamp) | ||
| if got.IsUpdateAvailable { | ||
| t.Errorf("CheckForUpdates() IsUpdateAvailable = %v, want false for dev builds", got.IsUpdateAvailable) | ||
| } | ||
| } | ||
|
|
||
| // TestCreateTopic_Integration tests topic creation with emulator | ||
| func TestCreateTopic_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "integration-test-topic" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Verify topic exists by getting metadata | ||
| topicInfo, err := app.GetTopicMetadata(topicID) | ||
| if err != nil { | ||
| t.Fatalf("GetTopicMetadata() error = %v", err) | ||
| } | ||
|
|
||
| if topicInfo.Name == "" { | ||
| t.Error("GetTopicMetadata() Name is empty") | ||
| } | ||
| func TestCheckForUpdates_UpdatesLastUpgradeCheck(t *testing.T) { | ||
| app := NewApp() | ||
| app.SetVersion("v1.0.0") | ||
|
|
||
| // Verify topic appears in list | ||
| topics, err := app.ListTopics() | ||
| // Initialize config manager and config | ||
| configMgr, err := config.NewManager() | ||
| if err != nil { | ||
| t.Fatalf("ListTopics() error = %v", err) | ||
| } | ||
|
|
||
| found := false | ||
| for _, topic := range topics { | ||
| if topic.Name == topicInfo.Name { | ||
| found = true | ||
| break | ||
| } | ||
| t.Fatalf("failed to create config manager: %v", err) | ||
| } | ||
| if !found { | ||
| t.Error("CreateTopic() topic not found in ListTopics()") | ||
| } | ||
| } | ||
|
|
||
| // TestDeleteTopic_Integration tests topic deletion with emulator | ||
| func TestDeleteTopic_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
| app.configManager = configMgr | ||
| app.config = models.NewDefaultConfig() | ||
|
|
||
| projectID := "test-project" | ||
| // Mock version.CheckForUpdates to return a result without making HTTP call | ||
| // Since we can't easily mock the version package, we'll test the timestamp update logic | ||
| initialTime := app.config.LastUpgradeCheck | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| // Note: This test would require mocking version.CheckForUpdates or making it testable | ||
| // For now, we verify the field exists and can be set | ||
| if !initialTime.IsZero() && !initialTime.Before(time.Now()) { | ||
| t.Error("initial LastUpgradeCheck should be zero or in the past") | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "integration-test-topic-delete" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
| // Verify the field can be updated | ||
| newTime := time.Now() | ||
| app.config.LastUpgradeCheck = newTime | ||
|
|
||
| // Delete topic | ||
| if err := app.DeleteTopic(topicID); err != nil { | ||
| t.Fatalf("DeleteTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Verify topic is deleted | ||
| _, err := app.GetTopicMetadata(topicID) | ||
| if err == nil { | ||
| t.Error("DeleteTopic() topic still exists after deletion") | ||
| if app.config.LastUpgradeCheck != newTime { | ||
| t.Errorf("LastUpgradeCheck = %v, want %v", app.config.LastUpgradeCheck, newTime) | ||
| } | ||
| } | ||
|
|
||
| // TestCreateSubscription_Integration tests subscription creation with emulator | ||
| func TestCreateSubscription_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "integration-test-topic-sub" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Create subscription | ||
| subID := "integration-test-sub" | ||
| ttlSeconds := int64(3600) // 1 hour | ||
| if err := app.CreateSubscription(topicID, subID, ttlSeconds); err != nil { | ||
| t.Fatalf("CreateSubscription() error = %v", err) | ||
| } | ||
|
|
||
| // Verify subscription exists | ||
| subInfo, err := app.GetSubscriptionMetadata(subID) | ||
| if err != nil { | ||
| t.Fatalf("GetSubscriptionMetadata() error = %v", err) | ||
| } | ||
|
|
||
| if subInfo.Name == "" { | ||
| t.Error("GetSubscriptionMetadata() Name is empty") | ||
| } | ||
|
|
||
| // Verify subscription appears in list | ||
| subscriptions, err := app.ListSubscriptions() | ||
| if err != nil { | ||
| t.Fatalf("ListSubscriptions() error = %v", err) | ||
| } | ||
|
|
||
| found := false | ||
| for _, sub := range subscriptions { | ||
| if sub.Name == subInfo.Name { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Error("CreateSubscription() subscription not found in ListSubscriptions()") | ||
| } | ||
| } | ||
|
|
||
| // TestDeleteSubscription_Integration tests subscription deletion with emulator | ||
| func TestDeleteSubscription_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic and subscription | ||
| topicID := "integration-test-topic-sub-delete" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| subID := "integration-test-sub-delete" | ||
| if err := app.CreateSubscription(topicID, subID, 3600); err != nil { | ||
| t.Fatalf("CreateSubscription() error = %v", err) | ||
| } | ||
| func TestCheckForUpdates_ErrorHandling(t *testing.T) { | ||
| app := NewApp() | ||
| app.SetVersion("v1.0.0") | ||
| app.config = models.NewDefaultConfig() | ||
|
|
||
| // Delete subscription | ||
| if err := app.DeleteSubscription(subID); err != nil { | ||
| t.Fatalf("DeleteSubscription() error = %v", err) | ||
| } | ||
| // Test with nil config manager (should not panic, just log warning) | ||
| app.configManager = nil | ||
|
|
||
| // Verify subscription is deleted | ||
| _, err := app.GetSubscriptionMetadata(subID) | ||
| // This should handle the nil config manager gracefully | ||
| // The actual version check will fail, but we test error handling | ||
| _, err := app.CheckForUpdates() | ||
| // Error is expected since we can't actually reach GitHub API in unit test | ||
| // We're just verifying it doesn't panic | ||
| if err == nil { | ||
| t.Error("DeleteSubscription() subscription still exists after deletion") | ||
| } | ||
| } | ||
|
|
||
| // TestStartMonitor_Integration tests message monitoring with emulator | ||
| func TestStartMonitor_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic and subscription | ||
| topicID := "integration-test-topic-monitor" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| subID := "integration-test-sub-monitor" | ||
| if err := app.CreateSubscription(topicID, subID, 3600); err != nil { | ||
| t.Fatalf("CreateSubscription() error = %v", err) | ||
| } | ||
|
|
||
| // Start monitoring | ||
| if err := app.StartMonitor(subID); err != nil { | ||
| t.Fatalf("StartMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Publish a message | ||
| payload := `{"test": "monitor"}` | ||
| if _, err := app.PublishMessage(topicID, payload, nil); err != nil { | ||
| t.Fatalf("PublishMessage() error = %v", err) | ||
| } | ||
|
|
||
| // Wait a bit for message to be received | ||
| time.Sleep(1 * time.Second) | ||
|
|
||
| // Get buffered messages | ||
| messages, err := app.GetBufferedMessages(subID) | ||
| if err != nil { | ||
| t.Fatalf("GetBufferedMessages() error = %v", err) | ||
| } | ||
|
|
||
| if len(messages) == 0 { | ||
| t.Error("StartMonitor() no messages received") | ||
| } else { | ||
| // Verify message content | ||
| msg := messages[0] | ||
| if msg.ID == "" { | ||
| t.Error("StartMonitor() message ID is empty") | ||
| } | ||
| if string(msg.Data) != payload { | ||
| t.Errorf("StartMonitor() message Data = %q, want %q", string(msg.Data), payload) | ||
| } | ||
| } | ||
|
|
||
| // Stop monitoring | ||
| if err := app.StopMonitor(subID); err != nil { | ||
| t.Fatalf("StopMonitor() error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TestStartTopicMonitor_Integration tests topic monitoring with auto-created subscription | ||
| func TestStartTopicMonitor_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "integration-test-topic-monitor-auto" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Start topic monitoring (auto-creates subscription) | ||
| if err := app.StartTopicMonitor(topicID, ""); err != nil { | ||
| t.Fatalf("StartTopicMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Publish a message | ||
| payload := `{"test": "topic-monitor"}` | ||
| if _, err := app.PublishMessage(topicID, payload, nil); err != nil { | ||
| t.Fatalf("PublishMessage() error = %v", err) | ||
| } | ||
|
|
||
| // Wait a bit for message to be received | ||
| time.Sleep(1 * time.Second) | ||
|
|
||
| // Verify subscription was created by listing subscriptions | ||
| subscriptions, err := app.ListSubscriptions() | ||
| if err != nil { | ||
| t.Fatalf("ListSubscriptions() error = %v", err) | ||
| } | ||
|
|
||
| // Verify at least one subscription exists (the auto-created one) | ||
| if len(subscriptions) == 0 { | ||
| t.Error("StartTopicMonitor() no subscription created") | ||
| } | ||
|
|
||
| // Stop topic monitoring (should clean up subscription) | ||
| if err := app.StopTopicMonitor(topicID); err != nil { | ||
| t.Fatalf("StopTopicMonitor() error = %v", err) | ||
| // If no error, that's fine - dev build or network succeeded | ||
| // We just want to ensure no panic | ||
| } | ||
| } | ||
|
|
||
| // TestDisconnect_Integration tests disconnect with active monitors | ||
| func TestDisconnect_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic and subscription | ||
| topicID := "integration-test-topic-disconnect" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| subID := "integration-test-sub-disconnect" | ||
| if err := app.CreateSubscription(topicID, subID, 3600); err != nil { | ||
| t.Fatalf("CreateSubscription() error = %v", err) | ||
| } | ||
|
|
||
| // Start monitoring | ||
| if err := app.StartMonitor(subID); err != nil { | ||
| t.Fatalf("StartMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Start topic monitoring | ||
| if err := app.StartTopicMonitor(topicID, ""); err != nil { | ||
| t.Fatalf("StartTopicMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Disconnect | ||
| if err := app.Disconnect(); err != nil { | ||
| t.Fatalf("Disconnect() error = %v", err) | ||
| } | ||
|
|
||
| // Verify connection is closed | ||
| status := app.GetConnectionStatus() | ||
| if status.IsConnected { | ||
| t.Error("Disconnect() IsConnected = true, want false") | ||
| } | ||
|
|
||
| // Verify resource cache is cleared | ||
| topics, err := app.ListTopics() | ||
| if err != nil { | ||
| t.Fatalf("ListTopics() error = %v", err) | ||
| } | ||
| if len(topics) != 0 { | ||
| t.Errorf("Disconnect() topics length = %d, want 0 (cache cleared)", len(topics)) | ||
| } | ||
| } | ||
|
|
||
| // TestStartup_Integration tests full startup sequence with emulator | ||
| func TestStartup_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| // Start emulator | ||
| _, cleanupEmulator := test.StartEmulator(t) | ||
| defer cleanupEmulator() | ||
|
|
||
| // Setup test config with active profile | ||
| configDir, cleanupConfig := test.SetupTestConfigDir(t) | ||
| defer cleanupConfig() | ||
|
|
||
| configPath := filepath.Join(configDir, ".pubsub-gui", "config.json") | ||
| configJSON := `{ | ||
| "profiles": [{ | ||
| "id": "test-profile", | ||
| "name": "Test Profile", | ||
| "projectId": "test-project", | ||
| "authMethod": "ADC", | ||
| "isDefault": true, | ||
| "createdAt": "2024-01-01T00:00:00Z" | ||
| }], | ||
| "activeProfileId": "test-profile", | ||
| "messageBufferSize": 500, | ||
| "autoAck": true, | ||
| "theme": "auto", | ||
| "fontSize": "medium", | ||
| "templates": [] | ||
| }` | ||
|
|
||
| if err := os.WriteFile(configPath, []byte(configJSON), 0644); err != nil { | ||
| t.Fatalf("failed to write config: %v", err) | ||
| } | ||
|
|
||
| // Create app and startup | ||
| func TestApp_UpgradeCheckFields(t *testing.T) { | ||
| app := NewApp() | ||
| ctx := context.Background() | ||
| app.startup(ctx) | ||
|
|
||
| // Verify handlers are initialized | ||
| if app.resources == nil { | ||
| t.Error("startup() resources handler is nil") | ||
| } | ||
| if app.connection == nil { | ||
| t.Error("startup() connection handler is nil") | ||
| } | ||
| if app.templates == nil { | ||
| t.Error("startup() templates handler is nil") | ||
| } | ||
| if app.monitoring == nil { | ||
| t.Error("startup() monitoring handler is nil") | ||
| } | ||
| if app.configH == nil { | ||
| t.Error("startup() config handler is nil") | ||
| } | ||
|
|
||
| // Verify config is loaded | ||
| if app.config == nil { | ||
| t.Error("startup() config is nil") | ||
| // Verify upgrade check fields are initialized | ||
| if app.upgradeCheckTicker != nil { | ||
| t.Error("upgradeCheckTicker should be nil initially") | ||
| } | ||
|
|
||
| // Verify auto-connect attempted (may succeed or fail, but should not crash) | ||
| status := app.GetConnectionStatus() | ||
| // Status may be connected or not, but should not panic | ||
| _ = status | ||
| // Verify mutex can be used | ||
| app.upgradeCheckMu.Lock() | ||
| app.lastUpgradeCheck = time.Now() | ||
| app.upgradeCheckMu.Unlock() | ||
|
|
||
| // Cleanup | ||
| app.Disconnect() | ||
| if app.lastUpgradeCheck.IsZero() { | ||
| t.Error("lastUpgradeCheck should be set after lock/unlock") | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: Complete replacement of integration tests with unit tests removes important test coverage. Integration tests validated real Pub/Sub operations (connection, topic/subscription CRUD, monitoring) which are core app functionality that should be preserved. Were the integration tests moved to another file, or are they permanently removed? The original tests covered critical Pub/Sub functionality.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app_test.go
Line: 1:139
Comment:
**logic:** Complete replacement of integration tests with unit tests removes important test coverage. Integration tests validated real Pub/Sub operations (connection, topic/subscription CRUD, monitoring) which are core app functionality that should be preserved. Were the integration tests moved to another file, or are they permanently removed? The original tests covered critical Pub/Sub functionality.
How can I resolve this? If you propose a fix, please make it concise.| // Mock version.CheckForUpdates to return a result without making HTTP call | ||
| // Since we can't easily mock the version package, we'll test the timestamp update logic | ||
| initialTime := app.config.LastUpgradeCheck | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| // Note: This test would require mocking version.CheckForUpdates or making it testable | ||
| // For now, we verify the field exists and can be set |
There was a problem hiding this comment.
style: Comments indicate the test is incomplete due to inability to mock the version package. Consider using dependency injection or interfaces to make this testable without external HTTP calls.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app_test.go
Line: 85:90
Comment:
**style:** Comments indicate the test is incomplete due to inability to mock the version package. Consider using dependency injection or interfaces to make this testable without external HTTP calls.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| app.configManager = nil | ||
|
|
||
| // Verify subscription is deleted | ||
| _, err := app.GetSubscriptionMetadata(subID) | ||
| // This should handle the nil config manager gracefully | ||
| // The actual version check will fail, but we test error handling | ||
| _, err := app.CheckForUpdates() | ||
| // Error is expected since we can't actually reach GitHub API in unit test | ||
| // We're just verifying it doesn't panic | ||
| if err == nil { | ||
| t.Error("DeleteSubscription() subscription still exists after deletion") | ||
| } | ||
| } | ||
|
|
||
| // TestStartMonitor_Integration tests message monitoring with emulator | ||
| func TestStartMonitor_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic and subscription | ||
| topicID := "integration-test-topic-monitor" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| subID := "integration-test-sub-monitor" | ||
| if err := app.CreateSubscription(topicID, subID, 3600); err != nil { | ||
| t.Fatalf("CreateSubscription() error = %v", err) | ||
| } | ||
|
|
||
| // Start monitoring | ||
| if err := app.StartMonitor(subID); err != nil { | ||
| t.Fatalf("StartMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Publish a message | ||
| payload := `{"test": "monitor"}` | ||
| if _, err := app.PublishMessage(topicID, payload, nil); err != nil { | ||
| t.Fatalf("PublishMessage() error = %v", err) | ||
| } | ||
|
|
||
| // Wait a bit for message to be received | ||
| time.Sleep(1 * time.Second) | ||
|
|
||
| // Get buffered messages | ||
| messages, err := app.GetBufferedMessages(subID) | ||
| if err != nil { | ||
| t.Fatalf("GetBufferedMessages() error = %v", err) | ||
| } | ||
|
|
||
| if len(messages) == 0 { | ||
| t.Error("StartMonitor() no messages received") | ||
| } else { | ||
| // Verify message content | ||
| msg := messages[0] | ||
| if msg.ID == "" { | ||
| t.Error("StartMonitor() message ID is empty") | ||
| } | ||
| if string(msg.Data) != payload { | ||
| t.Errorf("StartMonitor() message Data = %q, want %q", string(msg.Data), payload) | ||
| } | ||
| } | ||
|
|
||
| // Stop monitoring | ||
| if err := app.StopMonitor(subID); err != nil { | ||
| t.Fatalf("StopMonitor() error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TestStartTopicMonitor_Integration tests topic monitoring with auto-created subscription | ||
| func TestStartTopicMonitor_Integration(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip("skipping integration test") | ||
| } | ||
|
|
||
| app, cleanup := setupIntegrationTestApp(t) | ||
| defer cleanup() | ||
|
|
||
| projectID := "test-project" | ||
|
|
||
| // Connect to emulator | ||
| if err := app.ConnectWithADC(projectID); err != nil { | ||
| t.Fatalf("ConnectWithADC() error = %v", err) | ||
| } | ||
|
|
||
| // Create topic | ||
| topicID := "integration-test-topic-monitor-auto" | ||
| if err := app.CreateTopic(topicID, ""); err != nil { | ||
| t.Fatalf("CreateTopic() error = %v", err) | ||
| } | ||
|
|
||
| // Start topic monitoring (auto-creates subscription) | ||
| if err := app.StartTopicMonitor(topicID, ""); err != nil { | ||
| t.Fatalf("StartTopicMonitor() error = %v", err) | ||
| } | ||
|
|
||
| // Publish a message | ||
| payload := `{"test": "topic-monitor"}` | ||
| if _, err := app.PublishMessage(topicID, payload, nil); err != nil { | ||
| t.Fatalf("PublishMessage() error = %v", err) | ||
| } | ||
|
|
||
| // Wait a bit for message to be received | ||
| time.Sleep(1 * time.Second) | ||
|
|
||
| // Verify subscription was created by listing subscriptions | ||
| subscriptions, err := app.ListSubscriptions() | ||
| if err != nil { | ||
| t.Fatalf("ListSubscriptions() error = %v", err) | ||
| } | ||
|
|
||
| // Verify at least one subscription exists (the auto-created one) | ||
| if len(subscriptions) == 0 { | ||
| t.Error("StartTopicMonitor() no subscription created") | ||
| } | ||
|
|
||
| // Stop topic monitoring (should clean up subscription) | ||
| if err := app.StopTopicMonitor(topicID); err != nil { | ||
| t.Fatalf("StopTopicMonitor() error = %v", err) | ||
| // If no error, that's fine - dev build or network succeeded | ||
| // We just want to ensure no panic | ||
| } |
There was a problem hiding this comment.
logic: Error handling test allows both success and error cases, making it unclear what's actually being validated. The test comment suggests it's just checking for no panic, but the assertion logic is confusing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app_test.go
Line: 110:120
Comment:
**logic:** Error handling test allows both success and error cases, making it unclear what's actually being validated. The test comment suggests it's just checking for no panic, but the assertion logic is confusing.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/version/checker.go (1)
13-18: Minor logic redundancy inisDevBuild.Line 15 checks
version == "dev"but this is already covered bystrings.HasPrefix(lower, "dev")on line 16. The explicit check is redundant.🔎 Suggested simplification
func isDevBuild(version string) bool { lower := strings.ToLower(version) - return version == "dev" || - strings.HasPrefix(lower, "dev") || + return strings.HasPrefix(lower, "dev") || strings.HasSuffix(lower, "-dev") }.cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.md (1)
141-143: Add language specifiers to fenced code blocks.Several code blocks are missing language specifiers, which affects syntax highlighting and documentation quality. Consider adding appropriate language identifiers (e.g.,
text,plaintext, or the actual language).Static analysis flagged lines 141, 187, 224, 310, 341, and 1823 as missing language specifiers.
Also applies to: 187-192, 224-234, 310-337, 341-353, 1823-1855
frontend/src/components/Settings/UpgradeTab.tsx (1)
258-293: Addaria-labelto the check button for better accessibility.The button contains dynamic content (spinner vs icon) and could benefit from an explicit aria-label for screen readers.
🔎 Suggested improvement
<button onClick={handleCheckNow} disabled={checking || saving} + aria-label={checking ? 'Checking for updates' : 'Check for updates now'} style={{ backgroundColor: 'var(--color-accent-primary)',Based on learnings, interactive components should have aria-label for accessibility.
app.go (2)
169-198: Consider extracting version synchronization to a single location.
CheckForUpdatessets the version in the version package (versionpkg.SetVersion(a.GetVersion())) before callingversionpkg.CheckForUpdates(). This creates a coupling where the version must be set before each check. Consider setting the version once during startup (inmain.goorstartup()) to avoid this repeated synchronization.func (a *App) CheckForUpdates() (*versionpkg.UpdateInfo, error) { - // Set version in version package so CheckForUpdates can use it - versionpkg.SetVersion(a.GetVersion()) - // Call version.CheckForUpdates() updateInfo, err := versionpkg.CheckForUpdates()Then in
startup()ormain.go:versionpkg.SetVersion(app.GetVersion())
366-374: Consider validating URL format inOpenReleasesPage.The function validates that URL is not empty, but doesn't validate that it's a proper URL or specifically a GitHub releases URL. Consider adding validation to prevent potential security issues from arbitrary URL opening.
🔎 Suggested improvement
+import ( + "net/url" + "strings" +) func (a *App) OpenReleasesPage(url string) error { if url == "" { return fmt.Errorf("URL cannot be empty") } + + // Validate URL format + parsedURL, err := url.Parse(url) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + + // Optionally restrict to GitHub releases URLs for security + if !strings.HasPrefix(parsedURL.Host, "github.com") { + return fmt.Errorf("URL must be a GitHub releases URL") + } + runtime.BrowserOpenURL(a.ctx, url) return nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.mdapp.gofrontend/src/components/Settings/UpgradeTab.tsxfrontend/src/components/UpgradeNotification.tsxinternal/models/connection.gointernal/version/checker.go
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/UpgradeNotification.tsx
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Rungo fmt ./...on all Go code before committing
Uselog.Printf()for debugging Go code
Use the race detector withgo test -race ./...to debug race conditions
Initialize resource slices to empty arrays instead ofnilto prevent race conditions
Always lock shared state witha.resourceMu.RLock()anda.resourceMu.RUnlock()when accessing resources
Use goroutines with proper cleanup via context cancellation (ctx.Done()) and channels for message streaming
Emit events to frontend usingruntime.EventsEmit(a.ctx, "event:name", payload)for real-time updates
**/*.go: Use PascalCase for exported Go methods (e.g.,GetConnectionStatus,PublishMessage) to follow Go conventions and ensure proper method exposure
Use camelCase for private Go helper methods (e.g.,connectWithProfile,findExistingMonitoringSubscription)
Always return errors from Go methods, even when they may be optional in some contexts
Use custom errors frominternal/models/errors.gofor domain errors and wrap errors with context usingfmt.Errorf
All return types from exposed methods must be JSON-serializable with properjsontags using camelCase for field names,omitemptyfor optional fields, andjson:"-"to exclude fields
Always use the stored context (a.ctx) for API calls instead of creating new contexts withcontext.Background()
Emit events to notify frontend of state changes usingruntime.EventsEmit(a.ctx, eventName, payload)withresource:actionformat and kebab-case event names
Usesync.RWMutexfor read-heavy shared state,RLock()for read operations,Lock()for write operations, and minimize lock duration by unlocking before expensive operations
Normalize resource IDs when needed by backend methods before calling admin functions that construct full paths internally - extract short ID from full resource name if provided
Trigger background resource synchronization after mutations usinggo a.syncResources()and emitresources:updatedevent whe...
Files:
internal/models/connection.gointernal/version/checker.goapp.go
internal/models/connection.go
📄 CodeRabbit inference engine (.cursor/rules/theme-system.mdc)
internal/models/connection.go: AppConfig struct must include Theme field with values: 'light', 'dark', 'auto', 'dracula', 'monokai' and FontSize field with values: 'small', 'medium', 'large'
Default theme configuration must be Theme: 'auto' and FontSize: 'medium' as defined in the AppConfig struct
Files:
internal/models/connection.go
internal/models/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/wails.mdc)
Define custom domain error types and reusable error variables in
internal/models/errors.gofor consistent error handling across the application
Files:
internal/models/connection.go
app.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define new Go methods that need to be exposed to the frontend on the
Appstruct inapp.go
app.go: Storecontext.Contextas a field (set instartup()), use handler structs to organize methods by domain, initialize managers and handlers instartup()lifecycle hook, and clean up resources inDisconnect()andOnShutdown
All public methods onAppstruct are automatically exposed to frontend via Wails bindings
Files:
app.go
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Never use hardcoded colors (hex, rgb values) in React components; use semantic CSS variables likevar(--color-bg-primary)instead
Test React components with all 5 themes (Auto, Dark, Light, Dracula, Monokai) before committing
Addaria-labelor visible text to all interactive elements for accessibility
UseuseMemofor expensive computations to optimize React component performance
Use@tanstack/react-virtualfor lists with 100+ items to implement virtual scrolling
UseuseTheme()hook to access theme and fontSize from ThemeContext in React components
Ensure keyboard navigation works (Tab, Enter, Escape) and focus states are visible in interactive components
**/*.tsx: Import Go methods from auto-generated Wails bindings inwailsjs/go/main/Appand call them as async functions
Set up backend event listeners usingEventsOn()inuseEffectwith empty dependency array, store unsubscribe functions, and call them in cleanup to prevent memory leaks
Use try/catch blocks for all async Wails method calls, clear state on error to prevent stale data, and display user-friendly error messages
UseuseRefto access latest state in event handlers without triggering unnecessary re-renders
UseuseMemoto memoize filtered results from full resource lists to avoid unnecessary recalculations and for instant local filtering without API calls
Pass full resource lists as props to child components and filter locally usinguseMemoinstead of making backend API calls for relationship queries
Listen forresources:updatedevents to receive synchronized resource lists and update state directly without calling manual load functions
Files:
frontend/src/components/Settings/UpgradeTab.tsx
frontend/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use EventsOn/EventsOff from
wailsjs/runtime/runtimeto listen to backend events; unsubscribe in useEffect cleanup
Files:
frontend/src/components/Settings/UpgradeTab.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import Go methods from
../wailsjs/go/main/Appafter runningwails devto generate bindings
**/*.{ts,tsx}: Use PascalCase for component names and file names (e.g.,TopicDetails.tsx,ConnectionDialog.tsx)
UseComponentNamePropsinterface naming convention for component props (e.g.,TopicDetailsProps)
Always import types fromtypes/index.tsand use TypeScript interfaces for component props
CRITICAL: Always use semantic CSS variables (var(--color-*)) for all color styling via inline styles or custom CSS classes - never use hardcoded colors
Use Tailwind CSS classes only as fallback for layout, spacing, and sizing when semantic styling isn't practical
CRITICAL: All user-facing errors must be displayed in the UI - never rely solely on console.error. Convert technical errors to user-friendly messages
CRITICAL: Alwaysawaitall async operations that update state. Never call async functions without awaiting them
When switching connections or profiles: immediately clear dependent state (resources, selections) before any async operations, then verify connection, then load resources
Prefer manual user actions (buttons) over automatic actions for operations that consume resources, create/modify resources, or start background processes
CRITICAL: Test all components visually with all 5 themes (Dark, Light, Dracula, Monokai, Auto) to ensure theme variables are working correctly
UseuseMemofor expensive computations anduseCallbackfor stable function references to optimize performance
Always use semantic HTML and add ARIA labels where needed to ensure accessibility
Unsubscribe from Wails events in useEffect cleanup functions using the unsubscribe callback returned by EventsOn
Import Wails methods using:import { MethodName } from '../wailsjs/go/main/App'and call with await
When passing complex objects to Wails methods, convert plain TypeScript objects usingmain.TypeName.createFrom()from Wails-generated classes
UseuseRefto track state values in ev...
Files:
frontend/src/components/Settings/UpgradeTab.tsx
frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react-tailwind.mdc)
frontend/src/components/**/*.tsx: Keep components focused and small. Extract reusable logic into custom hooks
One component per file. File name must match component name in PascalCase
frontend/src/components/**/*.tsx: React components should NOT hardcode theme-specific color values; instead use semantic CSS variables (var(--color-bg-primary), etc.) or backward-compatible Tailwind classes
When accessing current theme in React components, use effectiveTheme (resolves 'auto' to actual theme) instead of theme for conditional logic that depends on actual theme values
Do not use useTheme() hook outside of ThemeProvider wrapped components as it will throw an error
Files:
frontend/src/components/Settings/UpgradeTab.tsx
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: React 18 with TypeScript is the framework for this desktop application. Wails v2 handles Go backend + React frontend integration
📚 Learning: 2026-01-06T22:29:17.712Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/theme-system.mdc:0-0
Timestamp: 2026-01-06T22:29:17.712Z
Learning: Applies to internal/models/connection.go : AppConfig struct must include Theme field with values: 'light', 'dark', 'auto', 'dracula', 'monokai' and FontSize field with values: 'small', 'medium', 'large'
Applied to files:
internal/models/connection.go
📚 Learning: 2026-01-06T22:29:17.712Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/theme-system.mdc:0-0
Timestamp: 2026-01-06T22:29:17.712Z
Learning: Applies to internal/models/connection.go : Default theme configuration must be Theme: 'auto' and FontSize: 'medium' as defined in the AppConfig struct
Applied to files:
internal/models/connection.go
📚 Learning: 2026-01-06T22:27:51.811Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T22:27:51.811Z
Learning: Applies to app.go : Define new Go methods that need to be exposed to the frontend on the `App` struct in `app.go`
Applied to files:
app.go
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to app.go : All public methods on `App` struct are automatically exposed to frontend via Wails bindings
Applied to files:
app.go
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to app.go : Store `context.Context` as a field (set in `startup()`), use handler structs to organize methods by domain, initialize managers and handlers in `startup()` lifecycle hook, and clean up resources in `Disconnect()` and `OnShutdown`
Applied to files:
app.go
📚 Learning: 2026-01-06T22:27:51.811Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T22:27:51.811Z
Learning: Applies to **/*.go : Use goroutines with proper cleanup via context cancellation (`ctx.Done()`) and channels for message streaming
Applied to files:
app.go
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to main.go : Use Wails lifecycle hooks (`OnStartup`, `OnShutdown`) for initialization and cleanup, storing context in `startup()` and cleaning up in `OnShutdown`
Applied to files:
app.go
📚 Learning: 2026-01-06T22:27:51.811Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T22:27:51.811Z
Learning: Applies to **/*.go : Emit events to frontend using `runtime.EventsEmit(a.ctx, "event:name", payload)` for real-time updates
Applied to files:
app.go
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to **/*.go : Emit events to notify frontend of state changes using `runtime.EventsEmit(a.ctx, eventName, payload)` with `resource:action` format and kebab-case event names
Applied to files:
app.go
📚 Learning: 2026-01-06T22:29:17.712Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/theme-system.mdc:0-0
Timestamp: 2026-01-06T22:29:17.712Z
Learning: Applies to internal/app/**/*.go : Config file location is ~/.pubsub-gui/config.json with permissions 0600 (user read/write only), and must be auto-created with defaults if missing
Applied to files:
app.go
📚 Learning: 2026-01-06T22:28:40.355Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: Applies to **/*.{ts,tsx} : Use Radix UI components (tabs, dialog, dropdown-menu, select, label, separator) for accessible, unstyled primitives. Style them with semantic CSS variables
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:27:51.811Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T22:27:51.811Z
Learning: Applies to **/*.tsx : Ensure keyboard navigation works (Tab, Enter, Escape) and focus states are visible in interactive components
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:28:40.355Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: Applies to **/*.{ts,tsx} : CRITICAL: Always `await` all async operations that update state. Never call async functions without awaiting them
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to **/*.tsx : Use try/catch blocks for all async Wails method calls, clear state on error to prevent stale data, and display user-friendly error messages
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:29:41.057Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/wails.mdc:0-0
Timestamp: 2026-01-06T22:29:41.057Z
Learning: Applies to **/*.ts : Use try/catch blocks for all async Wails method calls, clear state on error to prevent stale data, and display user-friendly error messages
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:28:40.355Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: Applies to **/*.{ts,tsx} : CRITICAL: All user-facing errors must be displayed in the UI - never rely solely on console.error. Convert technical errors to user-friendly messages
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:28:40.355Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: Applies to **/*.{ts,tsx} : Prefer manual user actions (buttons) over automatic actions for operations that consume resources, create/modify resources, or start background processes
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
📚 Learning: 2026-01-06T22:28:40.355Z
Learnt from: CR
Repo: B87/pubsub-gui PR: 0
File: .cursor/rules/react-tailwind.mdc:0-0
Timestamp: 2026-01-06T22:28:40.355Z
Learning: Applies to **/*.{ts,tsx} : CRITICAL: Verify connection status before loading resources after switching connections or profiles using `GetConnectionStatus()`
Applied to files:
frontend/src/components/Settings/UpgradeTab.tsx
🧬 Code graph analysis (2)
internal/version/checker.go (5)
frontend/wailsjs/go/main/App.js (2)
CheckForUpdates(5-7)GetVersion(93-95)frontend/src/types/upgrade.ts (1)
UpdateInfo(3-10)internal/version/models.go (1)
UpdateInfo(18-25)internal/version/version.go (1)
GetVersion(29-34)internal/version/github.go (1)
FetchLatestRelease(14-55)
app.go (6)
frontend/wailsjs/go/main/App.js (4)
StartPeriodicUpgradeCheck(137-139)GetVersion(93-95)CheckForUpdates(5-7)SetVersion(129-131)internal/version/version.go (2)
GetVersion(29-34)SetVersion(21-25)internal/version/checker.go (1)
CheckForUpdates(23-69)frontend/wailsjs/go/models.ts (1)
UpdateInfo(254-275)internal/version/models.go (1)
UpdateInfo(18-25)frontend/wailsjs/runtime/runtime.js (2)
EventsEmit(59-62)BrowserOpenURL(180-182)
🪛 markdownlint-cli2 (0.18.1)
.cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.md
141-141: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
224-224: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
341-341: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1823-1823: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (macos-15, darwin/arm64)
- GitHub Check: Build (ubuntu-latest, linux/amd64)
- GitHub Check: Build (windows-latest, windows/amd64)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (14)
internal/version/checker.go (2)
23-69: Well-structured update check implementation.The
CheckForUpdatesfunction correctly:
- Handles dev builds by returning early with no update available
- Normalizes versions before comparison
- Uses hashicorp/go-version for proper semantic version comparison
- Provides context-rich error messages for failures
- Returns complete
UpdateInfowith all relevant fields
71-78: LGTM!The
normalizeVersionfunction correctly handles both 'v' and 'V' prefixes and trims whitespace.internal/models/connection.go (1)
70-85: LGTM!The
NewDefaultConfigfunction correctly initializes all new upgrade-related fields with sensible defaults:
AutoCheckUpgrades: trueenables automatic checks by defaultUpgradeCheckInterval: 24(hours) is a reasonable defaultLastUpgradeCheck: time.Time{}(zero value) correctly indicates no previous checkDismissedUpgradeVersion: ""correctly indicates no dismissed versionBased on learnings, Theme and FontSize defaults are also correct (
autoandmedium)..cursor/tasks/TOPIC_SUBSCRIPTION_TEMPLATES.md (2)
1-5: This is an implementation plan document, not production code.This file is a detailed planning/specification document for a future template system feature. The code examples within are illustrative and not executed. Review comments focus on documentation quality.
1317-1323: Nil pointer dereference risk has been addressed.The code now correctly checks
overrides == nil || !overrides.DisableDeadLetterat line 1323, which addresses the previously flagged nil pointer dereference concern. The local variable assignment at line 1318 (overrides := req.Overrides) combined with the nil-safe check provides proper protection.frontend/src/components/Settings/UpgradeTab.tsx (4)
27-31: Consider adding cleanup for settings load.The
useEffectcallsloadSettings()andloadCurrentVersion()on mount, but these are fire-and-forget async calls. While this works, the pattern of calling async functions without tracking their promises in useEffect is fine here since they handle their own state updates and errors.
88-108: Error handling has been properly implemented.The
handleAutoCheckChangefunction now includes:
- Optimistic UI update with state revert on error
- try/catch wrapping async Wails calls
- User-friendly error messages displayed in UI
- Console logging for debugging
This addresses the previously flagged concern.
110-130: Error handling has been properly implemented.The
handleIntervalChangefunction now includes proper error handling with optimistic updates and revert on failure, addressing the previously flagged concern.
156-360: LGTM - Component follows theming guidelines.The component correctly uses semantic CSS variables throughout (
var(--color-bg-primary),var(--color-text-primary),var(--color-accent-primary), etc.) instead of hardcoded colors, which aligns with the coding guidelines. The UI provides clear feedback for all states (loading, checking, success, error).app.go (5)
47-54: LGTM - Proper fields for upgrade check lifecycle management.The new fields provide proper state management for the periodic upgrade check mechanism:
upgradeCheckMufor thread-safe accessupgradeCheckTickerandupgradeCheckTimerfor schedulingupgradeCheckDonechannel for clean goroutine shutdown
140-143: LGTM - Periodic upgrade check integrated into startup.The
StartPeriodicUpgradeCheckis appropriately called during startup, after config is loaded and handlers are initialized.
237-314: Goroutine leak issue has been properly addressed.The implementation now correctly handles goroutine lifecycle:
- Stops existing ticker and timer before creating new ones (lines 244-253)
- Uses a done channel with safe close pattern to signal goroutine exit (lines 257-264)
- The periodic goroutine uses select to listen for both done signal and ticker (lines 301-311)
- Local variable
doneis captured in closure to avoid race conditions (line 270)The safe close pattern using select prevents panic if the channel is already closed.
316-343: LGTM - Silent failure is appropriate for background checks.The
performUpgradeCheckfunction silently logs errors instead of notifying users, which is appropriate for background periodic checks. The dismissed version check correctly prevents repeated notifications for the same version.
418-438: LGTM - Proper cleanup in Disconnect.The cleanup logic correctly:
- Stops ticker and timer with nil assignment
- Safely closes the done channel using select to avoid panic
- Sets done channel to nil after closing
This ensures clean shutdown without goroutine leaks.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.