Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Discord Rich Presence (backend dependency, Tauri command, IPC client, frontend helpers and auto-sync), introduces managed thirdparty "modules" handling and manifest updates, changes default windowMaterial to "none", removes lazy-route helper, and applies multiple UI/card layout refactors. Changes
Sequence DiagramsequenceDiagram
participant App as Frontend App
participant Main as main.tsx
participant Store as Config/Connection Stores
participant TauriBridge as src/lib/tauri.ts
participant Backend as Tauri Backend
participant Discord as Discord IPC Client
App->>Main: start / route or state change
Main->>Store: read config, pathname, connection status
Main->>TauriBridge: syncDiscordPresence(enabled, details, state, activityType)
TauriBridge->>Backend: invoke sync_discord_presence
alt enabled == false
Backend->>Discord: clear presence and close client
Backend-->>TauriBridge: return true
else
Backend->>Backend: compute next_key vs last_key
alt key unchanged
Backend-->>TauriBridge: return true
else
Backend->>Discord: connect if needed, set_activity(activity)
alt set_activity success
Backend->>Backend: update last_key
Backend-->>TauriBridge: return true
else
Backend->>Discord: clear/close, reconnect, retry set_activity
alt retry success
Backend->>Backend: update last_key
Backend-->>TauriBridge: return true
else
Backend-->>TauriBridge: return false
end
end
end
end
Note over Main,Store: subscriptions trigger sync on route/config/status changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b3deacccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lastDiscordPresenceKey = nextKey | ||
| void tauri.syncDiscordPresence(enabled, details, state).catch((error) => { | ||
| console.error('Failed to sync Discord presence:', error) |
There was a problem hiding this comment.
This caches lastDiscordPresenceKey before knowing whether the Tauri command actually applied activity, so a failed/ignored sync can permanently suppress retries for the same route/status. A reproducible case is starting the app while Discord is closed: the backend returns without setting activity, but the key is still cached, so opening Discord later will not show presence until some other state change modifies the key.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/routes/strategies.tsx (1)
1-9: De-lazification is fine, but note the tradeoff. Switching/strategiesto a statically imported component removes code-splitting for this route, pullingCategoriesListPage(and its transitive deps) into the initial bundle. Acceptable if route-level splitting is no longer desired app-wide (consistent with the deletion oflazy-route.tsx), but considercreateLazyRoutefrom TanStack Router if you want to preserve splitting with the new routing APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/strategies.tsx` around lines 1 - 9, The current Route uses createRoute with a static import of CategoriesListPage which removes route-level code-splitting; to restore lazy-loading replace createRoute with createLazyRoute from '@tanstack/react-router' and provide a loader that dynamically imports the CategoriesListPage (instead of the static import), keeping getParentRoute: () => rootRoute and path: '/strategies' the same; refer to the Route constant, CategoriesListPage symbol, and the deleted lazy-route.tsx as the pattern to implement dynamic import so the component and its transitive deps are split from the initial bundle.src/components/features/AboutPage.tsx (2)
103-107: Prefercn()for conditional class composition.The codebase already uses
cn()from@/lib/utilseverywhere else (including the siblingSettingsSectionHeaderat line 89 ofSettingsPage.tsx). The manual[...].join(' ').trim()here is equivalent but inconsistent — please switch tocn()for uniformity and better dedup/merge behavior:- <CardHeader className={[ - '!flex !flex-row !items-center !gap-3 !p-4', - withDivider ? 'border-b border-border/60' : '', - ].join(' ').trim()} - > + <CardHeader + className={cn( + '!flex !flex-row !items-center !gap-3 !p-4', + withDivider && 'border-b border-border/60', + )} + >(
cnis already imported elsewhere in this project; addimport { cn } from '@/lib/utils'if not already present in this file.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/AboutPage.tsx` around lines 103 - 107, The CardHeader className is built with an array joined and trimmed instead of using the project's cn helper; replace the manual concatenation with cn() to compose classes and conditionally include 'border-b border-border/60' when withDivider is true, and ensure you import cn from '@/lib/utils' if it's not already imported; update the className prop on CardHeader (the JSX using CardHeader and the withDivider prop) to use cn([...]) or cn('!flex !flex-row !items-center !gap-3 !p-4', withDivider && 'border-b border-border/60') for consistent dedup/merge behavior.
89-118: Inconsistenticonprop typing vs. the other section-header helpers.
AboutSectionHeader(andMetaItemat line 120) typeiconastypeof Package, while the analogousSettingsSectionHeader/ModuleSectionHeaderin sibling files useReact.ComponentType<{ className?: string }>. It works today because every lucide-react icon happens to have the same type, but:
- It couples the prop type to a specific icon that may be renamed/removed.
- If this ever gets extracted into the shared
module-card.tsxsuggested forDnsPage.tsx, the structuralComponentTypeform is the right choice.- icon: typeof Package + icon: React.ComponentType<{ className?: string }>Apply the same change to
MetaItem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/AboutPage.tsx` around lines 89 - 118, AboutSectionHeader and MetaItem currently type their icon prop as typeof Package which ties the prop to a specific export; change both icon prop types to a structural React.ComponentType<{ className?: string }> (i.e. icon: React.ComponentType<{ className?: string }>) so any icon component can be passed safely. Locate AboutSectionHeader and MetaItem in this file, update their props/type annotations and any usages if necessary to accept and render the component with a className prop (e.g., <Icon className="..." />).src/lib/discord-presence.ts (1)
14-52: Consider a table-driven route → label map.The long
ifchain is fine functionally, but it means every new route/module page needs two edits (route file + this file) and there's nothing compile-time linking them. ARecord<string, string>lookup plus the single dynamic/strategies/<id>branch would trim this to ~10 lines and make stale entries easier to spot. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/discord-presence.ts` around lines 14 - 52, The long if-chain in getDiscordPresenceDetails should be replaced by a table-driven lookup: create a Record<string,string> (routeToLabel) mapping static paths to their labels and return routeToLabel[pathname] when present; keep the special-case logic for pathname.startsWith('/strategies/') using safeDecodePathSegment and config?.categories.find(category => category.id === categoryId)?.name to build `Стратегии: ${categoryName}` or fallback to 'Стратегии'; ensure the default fallback remains 'Zapret Interactive' and update references to getDiscordPresenceDetails and safeDecodePathSegment accordingly.src/main.tsx (2)
44-44:lastDiscordPresenceKeyisn’t reset by the HMR cleanup.The module-level
lastDiscordPresenceKeylives in the module closure; when the HMR replacement re-runs this module, a freshnullis used (so functionally fine). However, the previous__zapretDiscordPresenceSyncCleanup__only unsubscribes — it doesn't reset the Rust-sidelast_key. That's intentional de-duplication, but if the new module computes a different pathname while the Rust state still matches the old key, the next IPC call will be skipped on the Rust side and the Discord activity may appear stale until the next actual change. Consider invalidating the Rust-side cache on cleanup (e.g., by callingsyncDiscordPresence(false, …)and letting a subsequent change re-establish it) or simply living with the edge case given it only matters in dev HMR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` at line 44, The HMR cleanup currently only unsubscribes in __zapretDiscordPresenceSyncCleanup__ but does not invalidate the Rust-side dedupe key, so keep lastDiscordPresenceKey in sync by invoking syncDiscordPresence(false, <current path or empty key>) from the cleanup to clear the Rust-side cache (or otherwise call syncDiscordPresence with a value that forces invalidation) and then proceed to unsubscribe and null out the module-level lastDiscordPresenceKey; reference lastDiscordPresenceKey, __zapretDiscordPresenceSyncCleanup__, and syncDiscordPresence when implementing the cleanup call.
40-92: Presence sync runs before the config has loaded; consider subscribing tostatuschanges in the store subscription too.Two small caveats worth a look:
syncDiscordPresenceState()at line 92 is invoked synchronously at module-eval time.useConfigStore.getState().configisnulluntilload()completes, so the initial call is a no-op — a first real sync happens only when the config arrives (which triggers the config subscription). That's fine, but keep in mind the Discord activity won't appear until something actually changes after the first config load.- The config subscription only re-syncs when the top-level
configreference changes (line 75). Since every setter inconfig.store.tsdoesset({ config: { ...config, ... } }), flippingdiscordPresenceEnabledreplaces the reference and the sync fires correctly — but if a future setter mutatesconfigin place, the toggle will silently stop reacting. Worth guarding against by comparing the specific fields you depend on (discordPresenceEnabled,categories) instead of the whole object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` around lines 40 - 92, The initial sync runs at module-eval and often returns early because useConfigStore.getState().config is null; modify the logic around syncDiscordPresenceState(), the useConfigStore.subscribe callback, and the initial invocation so the first real sync runs when config becomes non-null (e.g., trigger sync when subscribe sees a transition from null -> config) and also make the config subscription compare the specific fields you depend on (discordPresenceEnabled and categories) instead of === on state.config; additionally subscribe to the connection status changes (or include status in the useConnectionStore subscribe you already have) so changes to connection status always call syncDiscordPresenceState(), and remove the module-eval-only reliance by not calling syncDiscordPresenceState() unconditionally at the bottom but only after one of those subscriptions detects a meaningful value change.src-tauri/src/commands/discord_presence.rs (1)
36-86: Globalstd::sync::Mutexis held across blocking Discord IPC calls.
presence_stateis locked for the entire duration ofconnect,set_activity,clear_activity, andclose— all of which perform synchronous IPC over a Unix socket / named pipe and can block for seconds if the Discord client is slow to respond (or not running — connect may time out). Because this is a#[tauri::command] pub fn(synchronous), it runs on Tauri's blocking worker pool, so the main async runtime isn't stalled; however, any concurrent invocation from the frontend will serialize behind this lock.Given the frontend already deduplicates via
lastDiscordPresenceKeyinsrc/main.tsx, concurrency pressure should be low in practice, so this is more of an operational note than a bug. If you ever start calling this from multiple surfaces (e.g. a tray menu), consider:
- Clone the
Option<DiscordIpcClient>out under the lock, operate without the lock held, then re-insert; or- Serialize via a dedicated worker thread + channel so command handlers never block on IPC.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/discord_presence.rs` around lines 36 - 86, The global Mutex (DISCORD_PRESENCE_STATE) is held across blocking IPC calls (connect/reconnect_client, client.set_activity, clear_presence, etc.), which serializes all concurrent Tauri commands; to fix, minimize lock duration by cloning or taking the Discord client and last_key under the lock, releasing the lock, performing blocking IPC (reconnect_client/connect or set_activity/clear_activity) on the cloned client or outside the lock, then reacquire the lock only to update presence_state.last_key or swap back the client; specifically, in functions using presence_state, grab a short-lived scope to extract presence_state.client (and presence_state.last_key when needed), call reconnect_client/clear_presence/set_activity without holding DISCORD_PRESENCE_STATE, and finally re-lock to store any updated client or last_key (adjust reconnect_client and clear_presence helpers to return the client or status so they can be used outside the lock).src/components/features/DnsPage.tsx (1)
27-80: ExtractModuleSectionHeader/ModuleSettingLabel/PAGE_CARD_CLASSinto a shared module.These helpers (and
PAGE_CARD_CLASS) are duplicated verbatim insrc/components/features/TgWsProxyPage.tsx, and the same visual primitive exists with slight variations inSettingsPage.tsx(SettingsSectionHeader/SettingLabel) andAboutPage.tsx(AboutSectionHeader). Consolidating them into e.g.src/components/features/_shared/module-card.tsxwill keep spacing, typography, and ring/border styling in lockstep and avoid drift as the design evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/DnsPage.tsx` around lines 27 - 80, Duplicate UI helpers (PAGE_CARD_CLASS, ModuleSectionHeader, ModuleSettingLabel) should be moved to a single shared module (e.g., module-card) and re-used: create a new file that exports PAGE_CARD_CLASS as a constant and named React components ModuleSectionHeader and ModuleSettingLabel with the exact same props/types and markup, then replace the local definitions in this file (and the matching copy in TgWsProxyPage) with imports from the shared module; keep the className/prop signatures unchanged so callers need only swap definitions for imports, and export the components as named exports for easy reuse by SettingsPage/AboutPage if you later consolidate their variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/Cargo.toml`:
- Line 52: The dependency line "discord-rich-presence = \"1.1.0\"" was added
manually to Cargo.toml; instead remove that manual edit and add the crate using
the Cargo tooling by running `cargo add discord-rich-presence` in the project
root (this will update Cargo.toml and Cargo.lock correctly), then run `cargo
check` to verify the workspace builds and dependency resolution; confirm the
manual line referencing discord-rich-presence in Cargo.toml is gone or matches
the cargo-added entry.
In `@src-tauri/src/commands/config.rs`:
- Around line 221-225: The config currently defaults discord_presence_enabled to
true via the default_discord_presence_enabled() function which will silently opt
existing users in; change the default to false by updating the
default_discord_presence_enabled() implementation to return false (or replace
the serde default to point to a new default function that returns false) and
update the persisted template in default-config.json to reflect false; also make
the same change for the other occurrence referenced by the reviewer (the other
default_discord_presence_enabled usage around lines 375-377) so both struct
fields and the shipped default config consistently opt users out by default.
In `@src-tauri/src/commands/discord_presence.rs`:
- Around line 34-50: The function sync_discord_presence currently swallows
errors from reconnect_client, making failures invisible; update both failure
branches (the initial reconnect in sync_discord_presence and the retry-path
where reconnect_client is called during retry logic) to log the error details
instead of just returning Ok(()): capture the Err returned by
reconnect_client(&mut presence_state), emit a diagnostic via tracing::warn! or
eprintln! including context (e.g., "reconnect_client failed in
sync_discord_presence" and the error), and then return Ok(()) as before so
behavior doesn't change but failures are visible; reference
DISCORD_PRESENCE_STATE, sync_discord_presence, and reconnect_client to locate
the two spots to change.
In `@src/lib/discord-presence.ts`:
- Line 3: ConnectionStatus is duplicated; extract it into a single shared
declaration and update imports so both the connection store and
getDiscordPresenceState use the same type. Either export the existing
ConnectionStatus from the connection store (and import it in
discord-presence.ts) or move the type to the shared types module (e.g.,
types.ts) and import it from there in both connection.store and
discord-presence; then adjust imports where ConnectionStatus is referenced
(including getDiscordPresenceState and any callers) so the compiler will catch
future added statuses.
---
Nitpick comments:
In `@src-tauri/src/commands/discord_presence.rs`:
- Around line 36-86: The global Mutex (DISCORD_PRESENCE_STATE) is held across
blocking IPC calls (connect/reconnect_client, client.set_activity,
clear_presence, etc.), which serializes all concurrent Tauri commands; to fix,
minimize lock duration by cloning or taking the Discord client and last_key
under the lock, releasing the lock, performing blocking IPC
(reconnect_client/connect or set_activity/clear_activity) on the cloned client
or outside the lock, then reacquire the lock only to update
presence_state.last_key or swap back the client; specifically, in functions
using presence_state, grab a short-lived scope to extract presence_state.client
(and presence_state.last_key when needed), call
reconnect_client/clear_presence/set_activity without holding
DISCORD_PRESENCE_STATE, and finally re-lock to store any updated client or
last_key (adjust reconnect_client and clear_presence helpers to return the
client or status so they can be used outside the lock).
In `@src/components/features/AboutPage.tsx`:
- Around line 103-107: The CardHeader className is built with an array joined
and trimmed instead of using the project's cn helper; replace the manual
concatenation with cn() to compose classes and conditionally include 'border-b
border-border/60' when withDivider is true, and ensure you import cn from
'@/lib/utils' if it's not already imported; update the className prop on
CardHeader (the JSX using CardHeader and the withDivider prop) to use cn([...])
or cn('!flex !flex-row !items-center !gap-3 !p-4', withDivider && 'border-b
border-border/60') for consistent dedup/merge behavior.
- Around line 89-118: AboutSectionHeader and MetaItem currently type their icon
prop as typeof Package which ties the prop to a specific export; change both
icon prop types to a structural React.ComponentType<{ className?: string }>
(i.e. icon: React.ComponentType<{ className?: string }>) so any icon component
can be passed safely. Locate AboutSectionHeader and MetaItem in this file,
update their props/type annotations and any usages if necessary to accept and
render the component with a className prop (e.g., <Icon className="..." />).
In `@src/components/features/DnsPage.tsx`:
- Around line 27-80: Duplicate UI helpers (PAGE_CARD_CLASS, ModuleSectionHeader,
ModuleSettingLabel) should be moved to a single shared module (e.g.,
module-card) and re-used: create a new file that exports PAGE_CARD_CLASS as a
constant and named React components ModuleSectionHeader and ModuleSettingLabel
with the exact same props/types and markup, then replace the local definitions
in this file (and the matching copy in TgWsProxyPage) with imports from the
shared module; keep the className/prop signatures unchanged so callers need only
swap definitions for imports, and export the components as named exports for
easy reuse by SettingsPage/AboutPage if you later consolidate their variants.
In `@src/lib/discord-presence.ts`:
- Around line 14-52: The long if-chain in getDiscordPresenceDetails should be
replaced by a table-driven lookup: create a Record<string,string> (routeToLabel)
mapping static paths to their labels and return routeToLabel[pathname] when
present; keep the special-case logic for pathname.startsWith('/strategies/')
using safeDecodePathSegment and config?.categories.find(category => category.id
=== categoryId)?.name to build `Стратегии: ${categoryName}` or fallback to
'Стратегии'; ensure the default fallback remains 'Zapret Interactive' and update
references to getDiscordPresenceDetails and safeDecodePathSegment accordingly.
In `@src/main.tsx`:
- Line 44: The HMR cleanup currently only unsubscribes in
__zapretDiscordPresenceSyncCleanup__ but does not invalidate the Rust-side
dedupe key, so keep lastDiscordPresenceKey in sync by invoking
syncDiscordPresence(false, <current path or empty key>) from the cleanup to
clear the Rust-side cache (or otherwise call syncDiscordPresence with a value
that forces invalidation) and then proceed to unsubscribe and null out the
module-level lastDiscordPresenceKey; reference lastDiscordPresenceKey,
__zapretDiscordPresenceSyncCleanup__, and syncDiscordPresence when implementing
the cleanup call.
- Around line 40-92: The initial sync runs at module-eval and often returns
early because useConfigStore.getState().config is null; modify the logic around
syncDiscordPresenceState(), the useConfigStore.subscribe callback, and the
initial invocation so the first real sync runs when config becomes non-null
(e.g., trigger sync when subscribe sees a transition from null -> config) and
also make the config subscription compare the specific fields you depend on
(discordPresenceEnabled and categories) instead of === on state.config;
additionally subscribe to the connection status changes (or include status in
the useConnectionStore subscribe you already have) so changes to connection
status always call syncDiscordPresenceState(), and remove the module-eval-only
reliance by not calling syncDiscordPresenceState() unconditionally at the bottom
but only after one of those subscriptions detects a meaningful value change.
In `@src/routes/strategies.tsx`:
- Around line 1-9: The current Route uses createRoute with a static import of
CategoriesListPage which removes route-level code-splitting; to restore
lazy-loading replace createRoute with createLazyRoute from
'@tanstack/react-router' and provide a loader that dynamically imports the
CategoriesListPage (instead of the static import), keeping getParentRoute: () =>
rootRoute and path: '/strategies' the same; refer to the Route constant,
CategoriesListPage symbol, and the deleted lazy-route.tsx as the pattern to
implement dynamic import so the component and its transitive deps are split from
the initial bundle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a639ecd-680f-45ef-bb91-94638f18c741
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
src-tauri/Cargo.tomlsrc-tauri/default-config.jsonsrc-tauri/src/commands/config.rssrc-tauri/src/commands/discord_presence.rssrc-tauri/src/commands/mod.rssrc-tauri/src/lib.rssrc/components/features/AboutPage.tsxsrc/components/features/CategoriesListPage.tsxsrc/components/features/CategoryPage.tsxsrc/components/features/DnsPage.tsxsrc/components/features/FiltersPage.tsxsrc/components/features/ModulesPage.tsxsrc/components/features/PlaceholdersPage.tsxsrc/components/features/SettingsPage.tsxsrc/components/features/TgWsProxyPage.tsxsrc/index.csssrc/lib/discord-presence.tssrc/lib/lazy-route.tsxsrc/lib/tauri.tssrc/lib/types.tssrc/main.tsxsrc/routes/filters.tsxsrc/routes/strategies.$categoryId.tsxsrc/routes/strategies.tsxsrc/stores/config.store.ts
💤 Files with no reviewable changes (2)
- src/index.css
- src/lib/lazy-route.tsx
| toml = "1.1.2" | ||
| duct = "1.1.1" | ||
| sysinfo = "0.38.4" | ||
| discord-rich-presence = "1.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify discord-rich-presence is available on crates.io
curl -s "https://crates.io/api/v1/crates/discord-rich-presence" | jq -r '.crate.max_version'Repository: Noktomezo/ZapretInteractive
Length of output: 76
Use cargo add discord-rich-presence instead of manually editing Cargo.toml.
Dependencies should be added using the cargo add command rather than manual editing. Add the dependency via cargo add discord-rich-presence and then run cargo check to verify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/Cargo.toml` at line 52, The dependency line "discord-rich-presence
= \"1.1.0\"" was added manually to Cargo.toml; instead remove that manual edit
and add the crate using the Cargo tooling by running `cargo add
discord-rich-presence` in the project root (this will update Cargo.toml and
Cargo.lock correctly), then run `cargo check` to verify the workspace builds and
dependency resolution; confirm the manual line referencing discord-rich-presence
in Cargo.toml is gone or matches the cargo-added entry.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd77cdd1c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (nextKey === lastDiscordPresenceKey) { | ||
| return |
There was a problem hiding this comment.
This early-return means the app never re-attempts IPC for the same enabled/details/state tuple, so if Discord is restarted (or its IPC pipe drops) while the user stays on the same page/status, presence can remain absent indefinitely until some unrelated state change alters the key. The cache should be invalidated or periodically retried so external Discord reconnects are reflected.
Useful? React with 👍 / 👎.
| group: "modules", | ||
| dest_path: get_modules_dir().join(name), | ||
| url: tracked_file_url("modules", name), | ||
| required_for_health: true, |
There was a problem hiding this comment.
Marking module executables as required_for_health makes overall binaries_ok fail when these files are missing/corrupt, and that flag is used to block the main connect flow (binariesOk === false) even when DNS/TG modules are disabled. Since these modules are optional features, treating them as critical can unnecessarily lock users out of core connectivity; health criticality should be conditional on module enablement or set to non-critical.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b7bb2e09b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let next_key = format!("{details}\u{0}{state}"); | ||
| if presence_state.last_key.as_deref() == Some(next_key.as_str()) { | ||
| return Ok(true); |
There was a problem hiding this comment.
The cache key here is built from only details and state, so changing discordPresenceActivityType while staying on the same route/connection state is treated as a no-op and returns before set_activity runs. In practice, switching from one activity type (e.g. playing) to another (e.g. listening) in Settings can report success but keep the old Discord activity type until some unrelated state change modifies details or state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/commands/config.rs (1)
160-167:⚠️ Potential issue | 🟡 MinorMove the enum default to
WindowMaterial::None.The enum derives
Defaultwith#[default]onAcrylic, but the serde default functiondefault_window_material()returnsNone. This creates a conflict whereWindowMaterial::default()producesAcrylicwhile deserialization defaults toNone. The enum's#[default]attribute should be moved to theNonevariant to align with the intended default behavior.Proposed fix
#[derive(Debug, Clone, Copy, Serialize, Default, PartialEq, Eq)] #[serde(rename_all = "lowercase")] pub enum WindowMaterial { + #[default] None, - #[default] Acrylic, Mica, Tabbed, }Optionally then simplify:
fn default_window_material() -> WindowMaterial { - WindowMaterial::None + WindowMaterial::default() }Also applies to: 406-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/config.rs` around lines 160 - 167, The Default/serde-default mismatch: move the #[default] attribute from the Acrylic variant to the None variant of the WindowMaterial enum so WindowMaterial::default() returns WindowMaterial::None to match the serde default function default_window_material(); update any other identical enum declarations (the second occurrence mentioned) the same way so both the derive(Default) behavior and serde deserialization default align.
🧹 Nitpick comments (1)
src-tauri/src/commands/binaries.rs (1)
179-181: Create the modules directory with the other base directories.
get_modules_dir()is now a managed resource root, butensure_base_directories()still initializes onlyfake,lists, andfilters. Creatingmodulesup front keeps clean-install layout and watcher setup consistent.♻️ Proposed fix
- for dir in [get_fake_dir(), get_lists_dir(), get_filters_dir()] { + for dir in [get_fake_dir(), get_lists_dir(), get_modules_dir(), get_filters_dir()] { if !dir.exists() { fs::create_dir_all(&dir).map_err(|e| e.to_string())?; } }Also applies to: 632-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/binaries.rs` around lines 179 - 181, The new managed resource root get_modules_dir() is not included in ensure_base_directories(), so the modules directory isn't created on clean install; update ensure_base_directories() (and any duplicate initialization block around the other occurrence noted) to create and initialize get_modules_dir() alongside the existing fake, lists, and filters directories—i.e., call get_modules_dir() and ensure its directory exists (matching the same creation logic used for get_fake_dir(), get_lists_dir(), get_filters_dir()) so the clean-install layout and watcher setup remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/update-thirdparty.py`:
- Around line 146-176: fetch_latest_release_asset and extract_zip_member
currently return the first matching item which can silently pick the wrong file
if multiple matches exist; change both to collect all matching assets/members
(e.g., build a list of matching asset dicts in fetch_latest_release_asset and
matching member names in extract_zip_member), then require exactly one match: if
len(matches) == 1 return that item (download the single asset or read the single
member), if len(matches) == 0 raise FileNotFoundError with context, and if
len(matches) > 1 raise a clear error (ValueError or FileExistsError) listing the
ambiguous matches and the patterns (use asset.get("name")/asset_url for assets
and the normalized member names for zip members) so the caller fails fast on
ambiguous matches.
In `@src-tauri/src/commands/binaries.rs`:
- Around line 381-390: The user-facing "empty-update" notification text still
only references winws/fake even though MODULE_FILES are now added to remote
updates via TrackedFile entries with include_in_remote_updates = true; update
the notification string (where the "no updates for winws/fake" message is
constructed) to also mention modules (or make it generic like "no updates found
for winws/fake or modules") so it reflects MODULE_FILES being included in remote
updates; locate the message near the remote-update/health-check code that
reports checked tracked files and adjust the text accordingly.
- Around line 681-690: The current watcher filter only checks exact tracked
destination paths (via tracked_files() -> tracked_dest_paths) so atomic writes
that emit parent-directory events are missed; modify the filter to also collect
each tracked file's parent directory (use dest_path.parent() for each entry)
into a tracked_parent_dirs set and update the paths.iter().any closure (the
path_is_inside check) to return true if the event path equals or is inside any
tracked_parent_dir in addition to the existing checks against filters_dir,
hashes_path, config_path, and tracked_dest_paths; update tests to assert that a
module replacement performed via a temp-file rename and deletion of a module
subdirectory both result in emitted files-health-changed events.
In `@src-tauri/src/commands/discord_presence.rs`:
- Around line 63-66: The de-duplication key built in the presence update
currently uses only details and state (next_key =
format!("{details}\u{0}{state}")), so changes to activity_type are ignored;
update the key construction to include the activity_type (e.g.,
format!("{activity_type}\u{0}{details}\u{0}{state}") or similar) and ensure the
comparison against presence_state.last_key.as_deref() uses that new key so
ActivityType changes no longer return early; locate and modify the next_key
formatting in the function handling presence updates in discord_presence.rs.
---
Outside diff comments:
In `@src-tauri/src/commands/config.rs`:
- Around line 160-167: The Default/serde-default mismatch: move the #[default]
attribute from the Acrylic variant to the None variant of the WindowMaterial
enum so WindowMaterial::default() returns WindowMaterial::None to match the
serde default function default_window_material(); update any other identical
enum declarations (the second occurrence mentioned) the same way so both the
derive(Default) behavior and serde deserialization default align.
---
Nitpick comments:
In `@src-tauri/src/commands/binaries.rs`:
- Around line 179-181: The new managed resource root get_modules_dir() is not
included in ensure_base_directories(), so the modules directory isn't created on
clean install; update ensure_base_directories() (and any duplicate
initialization block around the other occurrence noted) to create and initialize
get_modules_dir() alongside the existing fake, lists, and filters
directories—i.e., call get_modules_dir() and ensure its directory exists
(matching the same creation logic used for get_fake_dir(), get_lists_dir(),
get_filters_dir()) so the clean-install layout and watcher setup remain
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3945a53-73ba-4772-bb0a-40b9b1166c4a
⛔ Files ignored due to path filters (1)
thirdparty/modules/dnscrypt-proxy/localhost.pemis excluded by!**/*.pem
📒 Files selected for processing (20)
scripts/update-thirdparty.pyscripts/verify-thirdparty-hashes.pysrc-tauri/default-config.jsonsrc-tauri/src/commands/binaries.rssrc-tauri/src/commands/config.rssrc-tauri/src/commands/discord_presence.rssrc/components/features/SettingsPage.tsxsrc/components/features/TitleBar.tsxsrc/lib/discord-presence.tssrc/lib/tauri.tssrc/lib/types.tssrc/main.tsxsrc/routes/__root.tsxsrc/stores/config.store.tssrc/stores/connection.store.tsthirdparty/hashes.jsonthirdparty/modules/dnscrypt-proxy/public-resolvers.mdthirdparty/modules/dnscrypt-proxy/public-resolvers.md.minisigthirdparty/modules/dnscrypt-proxy/relays.mdthirdparty/modules/dnscrypt-proxy/relays.md.minisig
💤 Files with no reviewable changes (3)
- thirdparty/modules/dnscrypt-proxy/public-resolvers.md.minisig
- thirdparty/modules/dnscrypt-proxy/relays.md.minisig
- thirdparty/modules/dnscrypt-proxy/relays.md
✅ Files skipped from review due to trivial changes (3)
- thirdparty/hashes.json
- src/stores/config.store.ts
- src/components/features/SettingsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/types.ts
- src-tauri/default-config.json
- src/main.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2e505224
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let tracked_parent_dirs: Vec<PathBuf> = tracked_dest_paths | ||
| .iter() | ||
| .filter_map(|path| path.parent().map(Path::to_path_buf)) | ||
| .collect(); |
There was a problem hiding this comment.
This change broadens event_affects_tracked_files to treat any path under any tracked file’s parent directory as relevant; since core binaries live directly in get_managed_resources_dir(), the managed root itself becomes a parent and unrelated file writes there now trigger the full repair/health path. In the watcher loop this can cause unnecessary integrity scans and repair attempts for benign temp/untracked files, so the predicate should be narrowed to explicit tracked files (or only the specific subdirectories that truly need recursive matching).
Useful? React with 👍 / 👎.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/features/ModulesPage.tsx (1)
33-55:⚠️ Potential issue | 🟡 MinorAvoid clipping multiline module descriptions.
h-[4.5rem]leaves too little vertical room once a description wraps; the current long descriptions can overflow on narrower windows. Match the other cards withmin-h-[4.5rem]unless the description is clamped/truncated.Suggested fix
- 'group flex h-[4.5rem] items-center gap-3 rounded-lg border bg-card px-4 py-3', + 'group flex min-h-[4.5rem] items-center gap-3 rounded-lg border bg-card px-4 py-3',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/ModulesPage.tsx` around lines 33 - 55, The module card container currently uses a fixed height ('group flex h-[4.5rem] items-center...') which causes long descriptions to overflow; change that class to use a minimum height instead (replace h-[4.5rem] with min-h-[4.5rem]) so the card can grow when the description wraps. Update the class string in the ModulesPage component where the module row is rendered (the element with class 'group flex h-[4.5rem] items-center gap-3 rounded-lg border bg-card px-4 py-3')—no other logic changes needed (leave openModule, title, icon handling intact).
🧹 Nitpick comments (3)
src/components/features/DnsPage.tsx (1)
29-80: Extract the shared module UI helpers.
ModuleSectionHeaderandModuleSettingLabelare duplicated withsrc/components/features/TgWsProxyPage.tsx. Moving them to a shared component keeps DNS/TG module surfaces from drifting.♻️ Suggested direction
-function ModuleSectionHeader(...) { - ... -} - -function ModuleSettingLabel(...) { - ... -} +import { ModuleSectionHeader, ModuleSettingLabel } from '@/components/features/module-section'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/DnsPage.tsx` around lines 29 - 80, ModuleSectionHeader and ModuleSettingLabel are duplicated and should be extracted into a shared component module: create a new shared component (e.g., ModuleUI or ModuleHelpers) that exports both named components with the same props/signatures (preserve Icon prop types, htmlFor, description, children, and JSX structure), then replace the local definitions in both locations by importing { ModuleSectionHeader, ModuleSettingLabel } from the new module; ensure all import paths are updated where these symbols are used (including the other file that currently duplicates them) and run the app/TS checks to confirm prop types and className usage remain correct.src-tauri/src/commands/binaries.rs (1)
1097-1102: Reusetracked_display_namehere instead of inlining the group-prefixing.This switch now mirrors
tracked_display_namelogic (lines 336‑343), but it only hasfile.phase+file.name— not aTrackedFile. If another group is ever added totracked_files()/tracked_file_url, it's easy to forget to update this site (it quietly falls through to_ => file.name.clone()and loses the prefix in downloaded-files reporting).Consider either:
- passing the original
TrackedFile(or its display name) through intoFileToDownloadand using it here, or- extracting a small
format_group_label(group, name)helper shared betweentracked_display_nameand this branch.Non‑blocking, but it removes one place to get out of sync next time
MODULE_FILES-style group is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/binaries.rs` around lines 1097 - 1102, The downloaded list creation duplicates the prefixing logic from tracked_display_name (used elsewhere) by matching on file.phase and file.name; to avoid drift, modify the code so this branch reuses tracked_display_name logic instead of inlining: either (A) change FileToDownload to carry the original TrackedFile or a precomputed display_name and use tracked_display_name(tracked_file) here, or (B) extract the prefixing into a small helper like format_group_label(group: &str, name: &str) (used by tracked_display_name and by the match that currently inspects file.phase/file.name) and call that helper to produce the downloaded entry; update references to file.phase/file.name to use the new field/helper so future group additions stay consistent.src-tauri/src/commands/discord_presence.rs (1)
49-128: LGTM, with an optional DRY refactor.Key correctness items all look good:
next_keynow includes{activity_type:?}so changes to activity type alone invalidate the cache.- Reconnect failures on both the initial connect and the retry path are logged through
log_reconnect_failure, then surfaced to the caller asOk(false)so the frontend won't cache that key.clear_presencecorrectly dropslast_keyand gracefully closes the client before the retry connect.Optional refactor:
activity::Activity::new().activity_type(...).details(...).state(...).buttons(...)is repeated verbatim for the initial attempt (lines 75‑82) and the retry (lines 109‑116). A helper function would eliminate the duplication and centralize future changes to the activity shape:♻️ Suggested helper
+fn build_activity( + activity_type: DiscordPresenceActivityType, + details: String, + state: String, +) -> activity::Activity<'static> { + activity::Activity::new() + .activity_type(map_activity_type(activity_type)) + .details(details) + .state(state) + .buttons(vec![activity::Button::new( + "Доступ в интернет", + DISCORD_GITHUB_URL, + )]) +}The builder accepts owned
Stringvalues fordetailsandstate, and the button uses static string literals, so this helper correctly returnsActivity<'static>for safe IPC serialization.Non‑blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/discord_presence.rs` around lines 49 - 128, Create a small helper function (e.g., build_discord_activity) that takes the details: String, state: String, and activity_type: DiscordPresenceActivityType and returns the constructed activity::Activity (with map_activity_type(activity_type), details, state, and the static button); then replace the two duplicated builder blocks inside sync_discord_presence with calls to build_discord_activity (use cloned or moved Strings as needed to satisfy ownership) to remove duplication and centralize the activity shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/features/AboutPage.tsx`:
- Line 87: The PAGE_CARD_CLASS constant uses the deprecated prefix important
modifier (!utility); update the class string in PAGE_CARD_CLASS to use Tailwind
v4 postfix important syntax by moving each leading exclamation mark to the end
of each utility (e.g., change "!gap-0" to "gap-0!", "!rounded-lg" to
"rounded-lg!", etc.), ensuring all occurrences (!border-border/60, !bg-card,
!py-0, !shadow-none, !backdrop-blur-none) are converted to their postfix
equivalents so the class string conforms to Tailwind v4 best practices.
In `@src/components/features/DnsPage.tsx`:
- Line 27: Update Tailwind v3-style important prefixes to v4 suffix syntax in
this file: replace any class tokens using the `!utility` form with `utility!`.
Specifically update the PAGE_CARD_CLASS constant (and the other occurrences
noted in the file) to move each leading `!` to the end of the utility (e.g.,
`!rounded-lg` -> `rounded-lg!`), and scan the rest of
src/components/features/DnsPage.tsx for other `!`-prefixed utilities (the
locations noted in the review) and convert them the same way so all important
modifiers use the `utility!` suffix.
In `@src/components/features/SettingsPage.tsx`:
- Line 86: The Tailwind important modifier is using v3 leading-bang syntax which
fails under Tailwind v4; update the class strings to move each leading "!" to
the end of the utility token for PAGE_CARD_CLASS (replace !gap-0 -> gap-0!,
!rounded-lg -> rounded-lg!, !border -> border!, !border-border/60 ->
border-border/60!, !bg-card -> bg-card!, !py-0 -> py-0!, !shadow-none ->
shadow-none!, !backdrop-blur-none -> backdrop-blur-none!), update
SettingsSectionHeader class list (change !flex, !flex-row, !items-center,
!gap-3, !p-4 to flex!, flex-row!, items-center!, gap-3!, p-4!), and update
CardContent occurrences (change !p-4 -> p-4!) so all important modifiers are
trailing-bang per Tailwind v4 rules.
In `@src/components/features/TgWsProxyPage.tsx`:
- Line 14: The Tailwind important-prefix syntax in PAGE_CARD_CLASS (and other
class-string constants in this file) is deprecated; update each utility that
starts with "!" to use the v4 trailing "!" marker instead (e.g., change "!gap-0"
to "gap-0!" and "!rounded-lg" to "rounded-lg!"), and apply the same change to
the other class string constants in TgWsProxyPage.tsx referenced in the review
so all leading "!" prefixed utilities become trailing "!" suffixed utilities.
---
Outside diff comments:
In `@src/components/features/ModulesPage.tsx`:
- Around line 33-55: The module card container currently uses a fixed height
('group flex h-[4.5rem] items-center...') which causes long descriptions to
overflow; change that class to use a minimum height instead (replace h-[4.5rem]
with min-h-[4.5rem]) so the card can grow when the description wraps. Update the
class string in the ModulesPage component where the module row is rendered (the
element with class 'group flex h-[4.5rem] items-center gap-3 rounded-lg border
bg-card px-4 py-3')—no other logic changes needed (leave openModule, title, icon
handling intact).
---
Nitpick comments:
In `@src-tauri/src/commands/binaries.rs`:
- Around line 1097-1102: The downloaded list creation duplicates the prefixing
logic from tracked_display_name (used elsewhere) by matching on file.phase and
file.name; to avoid drift, modify the code so this branch reuses
tracked_display_name logic instead of inlining: either (A) change FileToDownload
to carry the original TrackedFile or a precomputed display_name and use
tracked_display_name(tracked_file) here, or (B) extract the prefixing into a
small helper like format_group_label(group: &str, name: &str) (used by
tracked_display_name and by the match that currently inspects
file.phase/file.name) and call that helper to produce the downloaded entry;
update references to file.phase/file.name to use the new field/helper so future
group additions stay consistent.
In `@src-tauri/src/commands/discord_presence.rs`:
- Around line 49-128: Create a small helper function (e.g.,
build_discord_activity) that takes the details: String, state: String, and
activity_type: DiscordPresenceActivityType and returns the constructed
activity::Activity (with map_activity_type(activity_type), details, state, and
the static button); then replace the two duplicated builder blocks inside
sync_discord_presence with calls to build_discord_activity (use cloned or moved
Strings as needed to satisfy ownership) to remove duplication and centralize the
activity shape.
In `@src/components/features/DnsPage.tsx`:
- Around line 29-80: ModuleSectionHeader and ModuleSettingLabel are duplicated
and should be extracted into a shared component module: create a new shared
component (e.g., ModuleUI or ModuleHelpers) that exports both named components
with the same props/signatures (preserve Icon prop types, htmlFor, description,
children, and JSX structure), then replace the local definitions in both
locations by importing { ModuleSectionHeader, ModuleSettingLabel } from the new
module; ensure all import paths are updated where these symbols are used
(including the other file that currently duplicates them) and run the app/TS
checks to confirm prop types and className usage remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d1ed8f5-8c73-480e-b6e9-f2903fa1e216
⛔ Files ignored due to path filters (2)
src-tauri/Cargo.lockis excluded by!**/*.lockthirdparty/modules/dnscrypt-proxy/localhost.pemis excluded by!**/*.pem
📒 Files selected for processing (36)
scripts/update-thirdparty.pyscripts/verify-thirdparty-hashes.pysrc-tauri/Cargo.tomlsrc-tauri/default-config.jsonsrc-tauri/src/commands/binaries.rssrc-tauri/src/commands/config.rssrc-tauri/src/commands/discord_presence.rssrc-tauri/src/commands/mod.rssrc-tauri/src/lib.rssrc/components/features/AboutPage.tsxsrc/components/features/CategoriesListPage.tsxsrc/components/features/CategoryPage.tsxsrc/components/features/DnsPage.tsxsrc/components/features/FiltersPage.tsxsrc/components/features/ModulesPage.tsxsrc/components/features/PlaceholdersPage.tsxsrc/components/features/SettingsPage.tsxsrc/components/features/TgWsProxyPage.tsxsrc/components/features/TitleBar.tsxsrc/index.csssrc/lib/discord-presence.tssrc/lib/lazy-route.tsxsrc/lib/tauri.tssrc/lib/types.tssrc/main.tsxsrc/routes/__root.tsxsrc/routes/filters.tsxsrc/routes/strategies.$categoryId.tsxsrc/routes/strategies.tsxsrc/stores/config.store.tssrc/stores/connection.store.tsthirdparty/hashes.jsonthirdparty/modules/dnscrypt-proxy/public-resolvers.mdthirdparty/modules/dnscrypt-proxy/public-resolvers.md.minisigthirdparty/modules/dnscrypt-proxy/relays.mdthirdparty/modules/dnscrypt-proxy/relays.md.minisig
💤 Files with no reviewable changes (5)
- thirdparty/modules/dnscrypt-proxy/relays.md.minisig
- src/lib/lazy-route.tsx
- src/index.css
- thirdparty/modules/dnscrypt-proxy/relays.md
- thirdparty/modules/dnscrypt-proxy/public-resolvers.md.minisig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63ba23458c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const synced = await tauri.syncDiscordPresence(enabled, details, state, activityType) | ||
| if (!enabled || synced) { | ||
| lastDiscordPresenceKey = nextKey | ||
| } |
There was a problem hiding this comment.
Fresh evidence in this commit: the cache is now updated only on synced, but when tauri.syncDiscordPresence(...) returns false this branch does nothing and no retry is scheduled. Because syncs are only triggered by route/config/connection changes, starting the app while Discord is closed and then opening Discord later (without changing page/status) leaves Rich Presence unsent indefinitely. Add a retry/backoff path for the synced === false case so recovery does not depend on unrelated UI state changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/features/AboutPage.tsx (1)
328-395:⚠️ Potential issue | 🟡 MinorHide the divider when the files card has no body.
AboutSectionHeaderdefaultswithDividertotrue, but this card omitsCardContentwhenshowBinaryDetailsis false, leaving a bottom border on a header-only card. Tie the divider to the same condition.Proposed fix
<AboutSectionHeader icon={FolderOpen} title={( <div className="flex items-center gap-2"> <span>Файлы приложения</span> @@ )} description="WinDivert, winws.exe, fake-файлы и списки" + withDivider={showBinaryDetails} action={(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/AboutPage.tsx` around lines 328 - 395, The AboutSectionHeader currently always renders with the divider (default withDivider=true) while the CardContent body is conditional on showBinaryDetails; change the AboutSectionHeader invocation to pass withDivider tied to the same condition (e.g., withDivider={showBinaryDetails}) so the divider is only shown when the CardContent (controlled by showBinaryDetails) is rendered; update the AboutSectionHeader props in the block that renders the files card to use that boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/features/TgWsProxyPage.tsx`:
- Around line 122-128: Update the ModuleSettingLabel for the secret field (the
instance with htmlFor="tg-ws-proxy-secret" and icon KeyRound) to include the
expected format in its description text (e.g., mention "32‑character hex" or
"32‑symbol hexadecimal (0-9a-f)") so users see the required format next to the
input; keep the visible hint in ModuleSettingLabel's description prop and ensure
any validation logic that disables the "Применить" button (the secret validity
check) still expects the same 32-char hex format so the UI hint matches the
validator.
---
Outside diff comments:
In `@src/components/features/AboutPage.tsx`:
- Around line 328-395: The AboutSectionHeader currently always renders with the
divider (default withDivider=true) while the CardContent body is conditional on
showBinaryDetails; change the AboutSectionHeader invocation to pass withDivider
tied to the same condition (e.g., withDivider={showBinaryDetails}) so the
divider is only shown when the CardContent (controlled by showBinaryDetails) is
rendered; update the AboutSectionHeader props in the block that renders the
files card to use that boolean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f6e076f-0e93-40dc-9a69-c54ad5fe4b69
📒 Files selected for processing (8)
src-tauri/src/commands/binaries.rssrc-tauri/src/commands/discord_presence.rssrc/components/features/AboutPage.tsxsrc/components/features/DnsPage.tsxsrc/components/features/ModulesPage.tsxsrc/components/features/SettingsPage.tsxsrc/components/features/TgWsProxyPage.tsxsrc/components/features/module-ui.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/features/ModulesPage.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/features/DnsPage.tsx
- src-tauri/src/commands/discord_presence.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main.tsx (2)
63-95: Subtle:nextKeyis computed at enqueue time but compared before awaiting.Because each call captures its own
nextKey/enabled/details/stateand chains ontodiscordPresenceSyncPromise, rapid successive state changes will each enqueue a Tauri call rather than coalescing to the latest. Ordering is preserved so the final state wins, but on slow IPC this can produce a burst of redundantsync_discord_presenceinvocations. If that matters, coalesce by checkingnextKey === lastDiscordPresenceKey(or a "latest requested key") inside the.thenright before callingtauri.syncDiscordPresence, and skip the call when a newer one has already been queued.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` around lines 63 - 95, The issue is that syncDiscordPresenceState computes nextKey at enqueue time so multiple rapid calls each schedule a tauri.syncDiscordPresence; to fix, record the requested key and re-check against the latest/last key inside the promise chain before making the IPC call: in syncDiscordPresenceState (and where discordPresenceSyncPromise is chained) compute nextKey as you do but also set a shared latestRequestedKey variable, then inside the .then handler (before calling tauri.syncDiscordPresence) recompute or read the current latestRequestedKey and compare to lastDiscordPresenceKey/this call's nextKey; if a newer key exists skip the tauri call and only update lastDiscordPresenceKey/clear retry when the effective latest was applied, otherwise proceed and update lastDiscordPresenceKey on success; refer to symbols nextKey, lastDiscordPresenceKey, latestRequestedKey (new), discordPresenceSyncPromise, tauri.syncDiscordPresence, scheduleDiscordPresenceRetry and clearDiscordPresenceRetryTimeout.
55-95: Unbounded fixed-interval retry may hammer the Tauri bridge.
scheduleDiscordPresenceRetryalways schedules after exactly 5s and has no cap on attempts. Iftauri.syncDiscordPresencepermanently rejects (e.g., Discord IPC socket unavailable), this will retry every 5s forever and log anconsole.erroreach time, plus the 30s heartbeat will pile more work onto the serialized promise chain. Consider exponential backoff with a max delay and/or a retry budget before giving up until the next config/route/status change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` around lines 55 - 95, The retry logic in scheduleDiscordPresenceRetry/syncDiscordPresenceState currently retries every 5s forever; change it to an exponential backoff with a capped max delay and a max attempt budget: add a retry counter (e.g., discordPresenceRetryAttempts) and make scheduleDiscordPresenceRetry accept/compute a delay via backoff (baseDelay * 2^attempts, capped at maxDelay) and increment the counter each schedule; stop scheduling further retries once attempts exceed maxAttempts or on success by clearing discordPresenceRetryTimeoutId and resetting discordPresenceRetryAttempts; ensure syncDiscordPresenceState resets attempts and lastDiscordPresenceKey on successful sync and also resets attempts when config/route/status changes so retries don’t persist across state changes; keep using discordPresenceSyncPromise for chaining but avoid piling by only scheduling new timeouts via scheduleDiscordPresenceRetry when attempts remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main.tsx`:
- Around line 63-95: The issue is that syncDiscordPresenceState computes nextKey
at enqueue time so multiple rapid calls each schedule a
tauri.syncDiscordPresence; to fix, record the requested key and re-check against
the latest/last key inside the promise chain before making the IPC call: in
syncDiscordPresenceState (and where discordPresenceSyncPromise is chained)
compute nextKey as you do but also set a shared latestRequestedKey variable,
then inside the .then handler (before calling tauri.syncDiscordPresence)
recompute or read the current latestRequestedKey and compare to
lastDiscordPresenceKey/this call's nextKey; if a newer key exists skip the tauri
call and only update lastDiscordPresenceKey/clear retry when the effective
latest was applied, otherwise proceed and update lastDiscordPresenceKey on
success; refer to symbols nextKey, lastDiscordPresenceKey, latestRequestedKey
(new), discordPresenceSyncPromise, tauri.syncDiscordPresence,
scheduleDiscordPresenceRetry and clearDiscordPresenceRetryTimeout.
- Around line 55-95: The retry logic in
scheduleDiscordPresenceRetry/syncDiscordPresenceState currently retries every 5s
forever; change it to an exponential backoff with a capped max delay and a max
attempt budget: add a retry counter (e.g., discordPresenceRetryAttempts) and
make scheduleDiscordPresenceRetry accept/compute a delay via backoff (baseDelay
* 2^attempts, capped at maxDelay) and increment the counter each schedule; stop
scheduling further retries once attempts exceed maxAttempts or on success by
clearing discordPresenceRetryTimeoutId and resetting
discordPresenceRetryAttempts; ensure syncDiscordPresenceState resets attempts
and lastDiscordPresenceKey on successful sync and also resets attempts when
config/route/status changes so retries don’t persist across state changes; keep
using discordPresenceSyncPromise for chaining but avoid piling by only
scheduling new timeouts via scheduleDiscordPresenceRetry when attempts remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da473076-ff74-4128-aef2-efd62fe11b0f
📒 Files selected for processing (3)
src-tauri/src/commands/binaries.rssrc/components/features/TgWsProxyPage.tsxsrc/main.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/commands/binaries.rs
- src/components/features/TgWsProxyPage.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db1b0df877
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if presence_state.last_key.as_deref() == Some(next_key.as_str()) { | ||
| return Ok(true); |
There was a problem hiding this comment.
This early return treats matching last_key as a successful sync before verifying the Discord IPC client is still usable. In practice, after Discord is restarted (or its IPC pipe drops), periodic frontend retries can call sync_discord_presence with the same payload, hit this branch, and never reconnect or re-send activity, leaving Rich Presence absent until some unrelated state changes the key.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Settings
UI/UX Improvements
Chores