Impl/autologout #3813
Impl/autologout #3813
Conversation
…n unit tests) (cherry picked from commit 33d57a2)
Number Component Does Not Allow Saving null Value shesha-io#3765
(cherry picked from commit d122f01) # Conflicts: # shesha-core/src/Shesha.Framework/Reflection/ReflectionHelper.cs
…ider to include AutoLogoutProvider and adjust settings in app settings and idle timer components.
WalkthroughAdds a backend flat-tree export API and DTOs, a warmup skip flag and guard, and numeric validation relaxation for nullable properties. Frontend introduces an AutoLogout provider (context, hooks, provider), switches config-items export from GraphQL to a REST flat-tree endpoint with package export, updates idle-timer usage, and tweaks number-field onChange handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ItemsExport UI
participant FE as React Client
participant API as ConfigurationItemAppService
participant Repo as Repository
Note over UI,API: Flat-tree export flow
UI->>FE: Select version mode
FE->>API: GET /api/services/app/ConfigurationItem/GetExportFlatTree?mode={1|2|3}
API->>Repo: Query items filtered by mode (Live/Ready/Latest)
Repo-->>API: Items
API-->>FE: 200 OK [flat items]
FE->>UI: Build/display tree
Note over UI,API: Export package
UI->>FE: Click Export (ids, dependencies)
FE->>API: POST /api/services/app/ConfigurationItem/ExportPackage { filter, exportDependencies, versionSelectionMode }
API-->>FE: 200 OK (blob + filename)
FE->>UI: Trigger download (package.zip)
sequenceDiagram
autonumber
participant Framework as SheshaFrameworkModule
participant Warmup as ApplicationWarmupService
participant BG as BackgroundWorker
participant Logger as Logger
Note over Framework,Warmup: App warmup guarded by skip flag
Warmup->>Framework: Check SkipAppWarmUp
alt SkipAppWarmUp == true
Warmup-->>Warmup: return immediately (skip warmup)
else
Warmup->>BG: Schedule ProcessAsync
BG-->>Warmup: Complete or throw
Warmup->>Logger: Log exceptions (if any)
end
sequenceDiagram
autonumber
participant App as AppProvider
participant ALP as AutoLogoutProvider
participant Auth as AuthService
participant User as User
Note over App,ALP: Auto-logout lifecycle
App->>ALP: Mount
ALP->>Auth: Check login state
Auth-->>ALP: IsLoggedIn?
ALP-->>ALP: Start per-second inactivity timer (if enabled)
User->>ALP: Activity events (mousemove, keypress, ...)
ALP-->>ALP: Reset lastActivity/timer
ALP-->>User: Show warning modal when threshold reached
User->>ALP: Keep Me Logged In / Logout Now
alt Keep Logged In
ALP-->>ALP: Reset timer
else Logout
ALP->>Auth: LogoutNow (and cleanup)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/components/idleTimerRenderer/index.tsx (1)
43-46: Duplicate idle handling vs new AutoLogoutProviderWith AutoLogoutProvider now wrapping the app and showing its own modal + countdown, this component will double-handle idleness and may trigger duplicate modals/logouts. Prefer deprecating this component or refactoring it to consume useAutoLogout and act as a pure presenter (or remove entirely).
I can draft a follow-up PR to migrate consumers to the provider and delete this component.
🧹 Nitpick comments (14)
shesha-reactjs/src/components/idleTimerRenderer/index.tsx (1)
81-106: Two overlapping modals create UX clashesThis modal duplicates the provider’s “Session Timeout Warning.” Keeping both risks confusing UX and race conditions on logout. Remove this modal once the provider is the single source of truth.
shesha-reactjs/src/providers/autoLogout/models.ts (1)
40-44: Consider clamping settings to safe boundsIf warningSeconds >= timeoutMinutes * 60, the warning will show immediately. Add simple clamping in the provider when merging settings to keep 1 ≤ warningSeconds < timeoutMinutes*60.
I can add a small helper to normalize settings.
shesha-reactjs/src/providers/autoLogout/contexts.ts (1)
24-24: Make context required by default to catch misusecreateContext with a concrete default makes useAutoLogout’s guard ineffective. Switch to undefined and let the hook enforce provider presence.
Apply:
-export const AutoLogoutContext = createContext<IAutoLogoutContext>(DEFAULT_CONTEXT_VALUE); +export const AutoLogoutContext = createContext<IAutoLogoutContext | undefined>(undefined);shesha-reactjs/src/providers/autoLogout/hooks.ts (1)
9-17: Enforce provider presence after contexts changeAfter switching the context to undefined-by-default, this guard will start working as intended. No additional changes needed, but ensure types reflect the possibly-undefined context.
Compile after applying the contexts change to confirm types flow.
Optionally, inline-narrow:
-const context = useContext(AutoLogoutContext); +const context = useContext(AutoLogoutContext); if (require && !context) { throw new Error('useAutoLogout must be used within an AutoLogoutProvider'); } return context!;shesha-reactjs/src/providers/autoLogout/provider.tsx (2)
37-47: Normalize settings when merging system/custom valuesIf custom warningSeconds ≥ timeoutMinutes*60, warning shows immediately. Clamp to a sane range while merging.
Example:
const getEffectiveSettings = useCallback((): IAutoLogoutSettings => { @@ - return { + const merged = { ...DEFAULT_AUTO_LOGOUT_SETTINGS, timeoutMinutes: systemTimeout, ...customSettings, }; + const maxSec = Math.max(1, merged.timeoutMinutes * 60); + return { + ...merged, + warningSeconds: Math.min(Math.max(1, merged.warningSeconds), Math.max(1, maxSec - 1)), + }; }, [securitySettings, customSettings]);
198-231: Consider adding visibilitychange to activity eventsWhen the tab becomes visible again, treat it as activity or at least re-check immediately. Minor UX polish.
- const events = ['mousedown', 'mousemove', 'keypress', 'scroll', 'touchstart', 'click']; + const events = ['mousedown', 'mousemove', 'keypress', 'scroll', 'touchstart', 'click', 'visibilitychange'];shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs (1)
46-47: Avoid mutable public flag; prefer config/options (and at least narrow the setter).A publicly settable boolean on a module is easy to flip at runtime and couples consumers to the module type. Consider binding this to configuration (IOptions or IConfiguration) and inject that into consumers, or at minimum restrict the setter to internal to reduce surface area.
Apply minimal hardening:
- public bool SkipAppWarmUp { get; set; } + public bool SkipAppWarmUp { get; internal set; }Option (preferred): introduce options and consume via DI in the warmup service (see related comment in ApplicationWarmupService).
shesha-core/src/Shesha.Framework/Warmup/ApplicationWarmupService.cs (2)
14-21: Decouple from module type; inject configuration/options instead.Injecting SheshaFrameworkModule into a service is a tight coupling and complicates testing. Prefer wiring via IConfiguration/IOptions (or a dedicated IWarmupOptions), which also avoids relying on module DI registration semantics.
Illustrative change:
- private readonly SheshaFrameworkModule _frameworkModule; + private readonly bool _skipWarmup; - public ApplicationWarmupService(IModelConfigurationManager entityConfigs, SheshaFrameworkModule frameworkModule) + public ApplicationWarmupService(IModelConfigurationManager entityConfigs, IConfiguration configuration) { _entityConfigs = entityConfigs; - _frameworkModule = frameworkModule; + _skipWarmup = configuration.GetValue<bool>("Shesha:SkipAppWarmUp", false); }And later:
if (_skipWarmup) { ... }
25-27: Log when skipping warmup for observability.Add an info/debug log so operators understand why warmup didn’t run.
- if (_frameworkModule.SkipAppWarmUp) - return Task.CompletedTask; + if (_frameworkModule.SkipAppWarmUp) + { + Logger.Info("Skipping application warmup (SkipAppWarmUp=true)."); + return Task.CompletedTask; + }shesha-core/src/Shesha.Application/ConfigurationItems/ConfigurationItemAppService.cs (3)
8-8: Remove unused importMicrosoft.Extensions.Options isn’t used in this file; drop it.
-using Microsoft.Extensions.Options;
326-326: Explicitly mark endpoint as GETRelying on conventions can be brittle; add [HttpGet] for clarity.
- public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode) + [HttpGet] + public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode)If your API conventions already map this to GET, feel free to skip; otherwise this prevents accidental POST routing.
356-382: Add AsNoTracking for read-only queryThis is a pure read; disable EF change tracking for better performance.
- var items = await query + var items = await query + .AsNoTracking() .OrderBy(e => e.Module!.Name) .ThenBy(e => e.Name)Note: ensure
using Microsoft.EntityFrameworkCore;is present at the top.shesha-reactjs/src/components/configurationFramework/itemsExport/index.tsx (2)
172-178: Reset loading state on errorsIf the request fails, isLoading stays true. Add a catch to reset it.
).then((response) => { applyItems(response.result); setIsLoading(false); - }); + }).catch(() => setIsLoading(false));
214-219: 'canExport' looks invertedReturning true when nothing is selected is counterintuitive for a property named canExport. If the consumer uses it to enable buttons, invert it.
- canExport: checkedIds.length === 0, + canExport: checkedIds.length > 0,If the consumer interprets canExport as “should disable export,” ignore this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
shesha-core/test/Shesha.Tests/SheshaTestModule.csis excluded by none and included by noneshesha-reactjs/package-lock.jsonis excluded by!**/package-lock.jsonand included by none
📒 Files selected for processing (14)
shesha-core/src/Shesha.Application/ConfigurationItems/ConfigurationItemAppService.cs(2 hunks)shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs(1 hunks)shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs(2 hunks)shesha-core/src/Shesha.Framework/Warmup/ApplicationWarmupService.cs(1 hunks)shesha-reactjs/src/app/app-provider.tsx(2 hunks)shesha-reactjs/src/components/configurationFramework/itemsExport/index.tsx(4 hunks)shesha-reactjs/src/components/idleTimerRenderer/index.tsx(1 hunks)shesha-reactjs/src/designer-components/numberField/numberField.tsx(1 hunks)shesha-reactjs/src/providers/autoLogout/contexts.ts(1 hunks)shesha-reactjs/src/providers/autoLogout/hooks.ts(1 hunks)shesha-reactjs/src/providers/autoLogout/index.ts(1 hunks)shesha-reactjs/src/providers/autoLogout/models.ts(1 hunks)shesha-reactjs/src/providers/autoLogout/provider.tsx(1 hunks)shesha-reactjs/src/providers/index.ts(1 hunks)
🔇 Additional comments (10)
shesha-reactjs/src/components/idleTimerRenderer/index.tsx (1)
30-36: Verify unit consistency for autoLogoffTimeout, remove debug log, and type the settings valueNo occurrences of
autoLogoffTimeoutorAutoLogoutProviderwere found; manually confirm whether the timeout is provided in minutes (and convert to seconds here), remove theconsole.log, and add a generic type touseSettingValue(e.g.<{ autoLogoffTimeout?: number }>) to document the settings shape.shesha-reactjs/src/providers/index.ts (1)
6-6: LGTM: public API extended cleanlyRe-exporting autoLogout from providers is straightforward and consistent.
shesha-reactjs/src/app/app-provider.tsx (1)
25-37: LGTM: Provider wired correctly; no IdleTimerRenderer references found
No legacy idle timer overlaps detected—no further action needed.shesha-reactjs/src/providers/autoLogout/index.ts (1)
1-4: LGTM: Barrel exports are tidyNamed export for provider and star-exports for hooks/models/contexts are fine.
shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs (1)
7-7: LGTM: required for IsNullable extension.The added using for Shesha.Reflection is appropriate for propInfo.IsNullable().
shesha-core/src/Shesha.Framework/Warmup/ApplicationWarmupService.cs (1)
17-21: Verify DI registration and absence of manual instantiation
No manualnew ApplicationWarmupService(...)calls or explicit registrations ofApplicationWarmupServiceorSheshaFrameworkModulewere found; confirm both are registered and resolvable by your DI container.shesha-reactjs/src/designer-components/numberField/numberField.tsx (1)
92-98: Unable to locatenumberField.tsxfor verification
I couldn’t findshesha-reactjs/src/designer-components/numberField/numberField.tsxor theonChangeInternalsnippet in the repo—please confirm the file path and lines 92–98 so the high-precision handling change can be properly reviewed.shesha-core/src/Shesha.Application/ConfigurationItems/ConfigurationItemAppService.cs (1)
326-384: ProtectGetExportFlatTreeAsyncwith authorization
Annotate this endpoint with the appropriate authorization attribute or insert an explicit permission check (e.g. usingAbpAuthorizeand the matching permission constant for exporting configuration items). Manually verify the correct permission constant name and attribute usage in your codebase.shesha-reactjs/src/components/configurationFramework/itemsExport/index.tsx (2)
56-63: Map versions to server enum names, not numeric literals
AligngetListFetcherQueryParamswith the server’sConfigurationItemViewModeenum to reduce coupling and improve readability:- const getListFetcherQueryParams = (mode: VerionSelectionMode): GetFlatTreePayload => { - switch(mode){ - default: - case 'live': return { mode: 1 }; - case 'ready': return { mode: 2 }; - case 'latest': return { mode: 3 }; - } - }; + const getListFetcherQueryParams = (mode: VerionSelectionMode): GetFlatTreePayload => { + switch (mode) { + case 'ready': return { mode: 'Ready' }; + case 'latest': return { mode: 'Latest' }; + case 'live': + default: return { mode: 'Live' }; + } + };Verify the server’s
ConfigurationItemViewModeenum definitions to confirm these string values match exactly.
197-199: Use consistent enum values for versionSelectionMode
Use the same mapping fromgetListFetcherQueryParamsto avoid sending lowercase strings:- versionSelectionMode: versionsMode, + versionSelectionMode: getListFetcherQueryParams(versionsMode).mode,Confirm that the server-side
PackageExportInput.versionSelectionModeexpects this enum value.
| public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode) | ||
| { | ||
| var query = Repository.GetAll(); | ||
| switch (mode) | ||
| { | ||
| case ConfigurationItemViewMode.Live: | ||
| query = query.Where(e => e.VersionStatus == ConfigurationItemVersionStatus.Live); | ||
| break; |
There was a problem hiding this comment.
Querying the wrong repository likely excludes most item types
Repository points to ConfigurationItem (the concrete type bound to the CRUD base), while export needs all ConfigurationItemBase derivatives. Use _itemsRepository to include all item types.
- public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode)
+ public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode)
{
- var query = Repository.GetAll();
+ var query = _itemsRepository.GetAll();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode) | |
| { | |
| var query = Repository.GetAll(); | |
| switch (mode) | |
| { | |
| case ConfigurationItemViewMode.Live: | |
| query = query.Where(e => e.VersionStatus == ConfigurationItemVersionStatus.Live); | |
| break; | |
| public async Task<List<ExportTreeFlatItem>> GetExportFlatTreeAsync(ConfigurationItemViewMode mode) | |
| { | |
| var query = _itemsRepository.GetAll(); | |
| switch (mode) | |
| { | |
| case ConfigurationItemViewMode.Live: | |
| query = query.Where(e => e.VersionStatus == ConfigurationItemVersionStatus.Live); | |
| break; |
🤖 Prompt for AI Agents
In
shesha-core/src/Shesha.Application/ConfigurationItems/ConfigurationItemAppService.cs
around lines 326 to 333, the method currently uses Repository (which is the
concrete ConfigurationItem repository) but the export must include all
ConfigurationItemBase-derived types; replace Repository.GetAll() with
_itemsRepository.GetAll() (or the equivalent items repository field) so the
query targets the polymorphic base set, then apply the same VersionStatus filter
and subsequent projections to that query.
| public bool SkipAppWarmUp { get; set; } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Where is SkipAppWarmUp set? Verify usage across the solution.
Ensure this flag is actually set from startup/config and not left at default false.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the flag is assigned or read
rg -nP '\bSkipAppWarmUp\b' -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Display the first 200 lines of SheshaFrameworkModule for inspection
sed -n '1,200p' shesha-core/src/Shesha.Framework/SheshaFrameworkModule.csLength of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate SheshaFrameworkModule.cs in the repo
fd --type f --name 'SheshaFrameworkModule.cs'
# Search for any assignments or usages of SkipAppWarmUp throughout the codebase
rg -nP '\bSkipAppWarmUp\b' -C2Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate SheshaFrameworkModule.cs in the repository
find . -type f -name '*SheshaFrameworkModule.cs'
# Search for any references or assignments of SkipAppWarmUp across the codebase
rg -n 'SkipAppWarmUp' -C2Length of output: 0
Wire up or remove SkipAppWarmUp flag
The SkipAppWarmUp property declared in SheshaFrameworkModule.cs (line 46) has no assignments or usages across the codebase. Either bind it from configuration and consume it in your startup logic, or remove this unused flag.
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs around lines 46-47,
the public bool SkipAppWarmUp property is declared but never used; either remove
it or wire it to configuration and use it to control startup warm-up. To fix,
choose one: 1) Remove the property and any dead-code references (if none exist)
to keep the module clean; or 2) Bind it from configuration (add IConfiguration
or IOptions<T> injection where modules are configured), read the value into the
module during initialization, and guard the app warm-up logic (where warm-up
tasks run in startup/bootstrap) with if (!SkipAppWarmUp) { ... } so the flag
actually controls whether warm-up runs.
| var stringValue = value?.ToString(); | ||
| if (string.IsNullOrWhiteSpace(stringValue) && propInfo != null && propInfo.IsNullable()) | ||
| return true; | ||
|
|
||
| var b = double.TryParse(stringValue, out double val); | ||
| if (!b) |
There was a problem hiding this comment.
Blank numeric strings can bypass Required; treat whitespace as null and enforce Required.
Current logic returns true for whitespace when the target property is nullable, but it doesn’t honor propConfig.Required for “blank” inputs (value != null). This can let required numeric fields pass as blank.
Apply:
- var stringValue = value?.ToString();
- if (string.IsNullOrWhiteSpace(stringValue) && propInfo != null && propInfo.IsNullable())
- return true;
-
- var b = double.TryParse(stringValue, out double val);
+ var stringValue = value?.ToString();
+ if (string.IsNullOrWhiteSpace(stringValue))
+ {
+ if (propConfig.Required && !propConfig.Suppress)
+ {
+ validationResult.Add(new ValidationResult(hasMessage
+ ? propConfig.ValidationMessage
+ : $"Property '{friendlyName}' is required."));
+ return false;
+ }
+ if (propInfo != null && propInfo.IsNullable())
+ return true;
+ }
+
+ var b = double.TryParse(stringValue, out double val);Optional: consider culture-aware parsing to avoid ambiguity with decimals/thousands (requires using System.Globalization;):
var b = double.TryParse(stringValue, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.CurrentCulture, out double val);🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs
around lines 162 to 167, the validator currently treats whitespace-only numeric
strings as null when the property is nullable, which allows required numeric
fields to bypass validation; change the logic so that if value is a
whitespace-only string it is treated as null only when value == null (i.e.,
preserve that blank input is still a provided value) and enforce
propConfig.Required for non-null blank inputs—specifically, do not return true
for whitespace when value != null and propConfig.Required is set; additionally,
replace the current double.TryParse with a culture-aware parse call (e.g., use
NumberStyles.Float | NumberStyles.AllowThousands and CultureInfo.CurrentCulture)
to reliably parse numeric input across locales.
| type GetFlatTreePayload = { | ||
| mode: number; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid magic ints; use enum names to decouple FE from BE numeric values
Map to 'Live' | 'Ready' | 'Latest' so a server enum rebase won’t break the client.
-type GetFlatTreePayload = {
- mode: number;
-};
+type GetFlatTreePayload = {
+ mode: 'Live' | 'Ready' | 'Latest';
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type GetFlatTreePayload = { | |
| mode: number; | |
| }; | |
| type GetFlatTreePayload = { | |
| mode: 'Live' | 'Ready' | 'Latest'; | |
| }; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/components/configurationFramework/itemsExport/index.tsx
around lines 41-44, replace the magic-typed "mode: number" with a named enum or
string union representing the server modes (e.g. 'Live' | 'Ready' | 'Latest'),
and update any code that constructs the payload to use these names; add a single
conversion layer that maps those names to the backend numeric values
(centralized constant or mapper function) right before sending the request so
the FE is decoupled from backend enum reordering.
| const timeoutRef = useRef<NodeJS.Timeout>(); | ||
|
|
||
| const getEffectiveSettings = useCallback((): IAutoLogoutSettings => { | ||
| const systemTimeout = securitySettings?.autoLogoffTimeout && securitySettings.autoLogoffTimeout > 0 | ||
| ? securitySettings.autoLogoffTimeout | ||
| : DEFAULT_AUTO_LOGOUT_SETTINGS.timeoutMinutes; | ||
|
|
||
| return { | ||
| ...DEFAULT_AUTO_LOGOUT_SETTINGS, | ||
| timeoutMinutes: systemTimeout, | ||
| ...customSettings, | ||
| }; | ||
| }, [securitySettings, customSettings]); | ||
|
|
||
| const [state, setState] = useState<IAutoLogoutState>(() => ({ | ||
| isWarningVisible: false, | ||
| remainingTime: 0, | ||
| isIdle: false, | ||
| settings: getEffectiveSettings(), | ||
| lastActivity: Date.now(), | ||
| })); | ||
|
|
||
| const clearTimers = useCallback(() => { | ||
| if (timeoutRef.current) { | ||
| clearInterval(timeoutRef.current); | ||
| timeoutRef.current = undefined; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Interval callback captures stale state (timer won’t honor activity resets)
setInterval holds the first checkInactivity closure, which reads state.lastActivity/isWarningVisible/settings from that render. After activity, the interval still uses stale values and may log users out incorrectly.
Refactor to read from refs and avoid stale closures; also use a DOM-safe timer type:
- const timeoutRef = useRef<NodeJS.Timeout>();
+ const intervalRef = useRef<ReturnType<typeof setInterval>>();
+ const lastActivityRef = useRef<number>(Date.now());
+ const isWarningVisibleRef = useRef<boolean>(false);
+ const settingsRef = useRef<IAutoLogoutSettings>(getEffectiveSettings());Keep refs in sync:
@@
const [state, setState] = useState<IAutoLogoutState>(() => ({
@@
}));
+
+ useEffect(() => {
+ lastActivityRef.current = state.lastActivity;
+ isWarningVisibleRef.current = state.isWarningVisible;
+ settingsRef.current = state.settings;
+ }, [state.lastActivity, state.isWarningVisible, state.settings]);Clear timers:
- if (timeoutRef.current) {
- clearInterval(timeoutRef.current);
- timeoutRef.current = undefined;
+ if (intervalRef.current) {
+ clearInterval(intervalRef.current);
+ intervalRef.current = undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeoutRef = useRef<NodeJS.Timeout>(); | |
| const getEffectiveSettings = useCallback((): IAutoLogoutSettings => { | |
| const systemTimeout = securitySettings?.autoLogoffTimeout && securitySettings.autoLogoffTimeout > 0 | |
| ? securitySettings.autoLogoffTimeout | |
| : DEFAULT_AUTO_LOGOUT_SETTINGS.timeoutMinutes; | |
| return { | |
| ...DEFAULT_AUTO_LOGOUT_SETTINGS, | |
| timeoutMinutes: systemTimeout, | |
| ...customSettings, | |
| }; | |
| }, [securitySettings, customSettings]); | |
| const [state, setState] = useState<IAutoLogoutState>(() => ({ | |
| isWarningVisible: false, | |
| remainingTime: 0, | |
| isIdle: false, | |
| settings: getEffectiveSettings(), | |
| lastActivity: Date.now(), | |
| })); | |
| const clearTimers = useCallback(() => { | |
| if (timeoutRef.current) { | |
| clearInterval(timeoutRef.current); | |
| timeoutRef.current = undefined; | |
| } | |
| }, []); | |
| // Replace timeoutRef and add refs for latest state values | |
| const intervalRef = useRef<ReturnType<typeof setInterval>>(); | |
| const lastActivityRef = useRef<number>(Date.now()); | |
| const isWarningVisibleRef = useRef<boolean>(false); | |
| const settingsRef = useRef<IAutoLogoutSettings>(getEffectiveSettings()); | |
| const getEffectiveSettings = useCallback((): IAutoLogoutSettings => { | |
| const systemTimeout = | |
| securitySettings?.autoLogoffTimeout && securitySettings.autoLogoffTimeout > 0 | |
| ? securitySettings.autoLogoffTimeout | |
| : DEFAULT_AUTO_LOGOUT_SETTINGS.timeoutMinutes; | |
| return { | |
| ...DEFAULT_AUTO_LOGOUT_SETTINGS, | |
| timeoutMinutes: systemTimeout, | |
| ...customSettings, | |
| }; | |
| }, [securitySettings, customSettings]); | |
| const [state, setState] = useState<IAutoLogoutState>(() => ({ | |
| isWarningVisible: false, | |
| remainingTime: 0, | |
| isIdle: false, | |
| settings: getEffectiveSettings(), | |
| lastActivity: Date.now(), | |
| })); | |
| // Sync refs whenever state changes | |
| useEffect(() => { | |
| lastActivityRef.current = state.lastActivity; | |
| isWarningVisibleRef.current = state.isWarningVisible; | |
| settingsRef.current = state.settings; | |
| }, [state.lastActivity, state.isWarningVisible, state.settings]); | |
| const clearTimers = useCallback(() => { | |
| if (intervalRef.current) { | |
| clearInterval(intervalRef.current); | |
| intervalRef.current = undefined; | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/autoLogout/provider.tsx around lines 35 to 62,
the interval callback closes over stale state (lastActivity, isWarningVisible,
settings) causing timers to ignore activity resets; replace state reads inside
the interval with refs by creating refs for lastActivity, settings and
isWarningVisible, update those refs whenever you set state (e.g. in the state
setter or effects) so they stay in sync, change timeoutRef to a DOM-safe type
(number | null) instead of NodeJS.Timeout, and modify clearTimers to clear the
window timer and set timeoutRef to null; ensure the interval callback reads from
the refs to determine remaining time and warning logic so activity resets are
honored.
| const recordActivity = useCallback(() => { | ||
| const now = Date.now(); | ||
| setState(prev => { | ||
| // Only update if there's actually been some time passed to avoid unnecessary re-renders | ||
| if (now - prev.lastActivity > 1000 || prev.isWarningVisible || prev.isIdle) { | ||
| return { | ||
| ...prev, | ||
| lastActivity: now, | ||
| isWarningVisible: false, | ||
| isIdle: false, | ||
| remainingTime: 0, | ||
| }; | ||
| } | ||
| return prev; | ||
| }); | ||
| onTimerReset?.(); | ||
| }, [onTimerReset]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update ref on activity to ensure immediate effect
Also update the ref in recordActivity to reflect the latest activity time.
- const recordActivity = useCallback(() => {
+ const recordActivity = useCallback(() => {
const now = Date.now();
+ lastActivityRef.current = now;
setState(prev => {
// Only update if there's actually been some time passed to avoid unnecessary re-renders
if (now - prev.lastActivity > 1000 || prev.isWarningVisible || prev.isIdle) {
return {
...prev,
lastActivity: now,
isWarningVisible: false,
isIdle: false,
remainingTime: 0,
};
}
return prev;
});
onTimerReset?.();
}, [onTimerReset]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const recordActivity = useCallback(() => { | |
| const now = Date.now(); | |
| setState(prev => { | |
| // Only update if there's actually been some time passed to avoid unnecessary re-renders | |
| if (now - prev.lastActivity > 1000 || prev.isWarningVisible || prev.isIdle) { | |
| return { | |
| ...prev, | |
| lastActivity: now, | |
| isWarningVisible: false, | |
| isIdle: false, | |
| remainingTime: 0, | |
| }; | |
| } | |
| return prev; | |
| }); | |
| onTimerReset?.(); | |
| }, [onTimerReset]); | |
| const recordActivity = useCallback(() => { | |
| const now = Date.now(); | |
| lastActivityRef.current = now; | |
| setState(prev => { | |
| // Only update if there's actually been some time passed to avoid unnecessary re-renders | |
| if (now - prev.lastActivity > 1000 || prev.isWarningVisible || prev.isIdle) { | |
| return { | |
| ...prev, | |
| lastActivity: now, | |
| isWarningVisible: false, | |
| isIdle: false, | |
| remainingTime: 0, | |
| }; | |
| } | |
| return prev; | |
| }); | |
| onTimerReset?.(); | |
| }, [onTimerReset]); |
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/autoLogout/provider.tsx around lines 84 to 101,
recordActivity updates component state but does not update the corresponding
activity ref, so external timers/readers still see stale activity; modify
recordActivity to set the ref to the new timestamp (e.g., activityRef.current =
now) immediately when computing now and before calling setState/onTimerReset so
the ref and state stay in sync, and ensure the useCallback dependency list
includes any ref setter or onTimerReset as needed.
| const checkInactivity = useCallback(() => { | ||
| if (!isLoggedIn || !state.settings.enabled) { | ||
| return; | ||
| } | ||
|
|
||
| const now = Date.now(); | ||
| const timeSinceActivity = now - state.lastActivity; | ||
| const inactiveSeconds = Math.floor(timeSinceActivity / 1000); | ||
| const maxInactiveSeconds = state.settings.timeoutMinutes * 60; | ||
| const warningThreshold = maxInactiveSeconds - state.settings.warningSeconds; | ||
|
|
||
| if (inactiveSeconds >= maxInactiveSeconds) { | ||
| // Time to logout | ||
| handleLogout(); | ||
| } else if (inactiveSeconds >= warningThreshold && !state.isWarningVisible) { | ||
| // Show warning | ||
| const remainingSeconds = maxInactiveSeconds - inactiveSeconds; | ||
| setState(prev => ({ | ||
| ...prev, | ||
| isWarningVisible: true, | ||
| isIdle: true, | ||
| remainingTime: remainingSeconds, | ||
| })); | ||
| onWarningShown?.(); | ||
| } else if (state.isWarningVisible) { | ||
| // Update countdown | ||
| const remainingSeconds = maxInactiveSeconds - inactiveSeconds; | ||
| setState(prev => ({ | ||
| ...prev, | ||
| remainingTime: Math.max(0, remainingSeconds), | ||
| })); | ||
| } | ||
| }, [isLoggedIn, state.settings, state.lastActivity, state.isWarningVisible, handleLogout, onWarningShown]); | ||
|
|
||
| const startTimer = useCallback(() => { | ||
| clearTimers(); | ||
|
|
||
| if (!isLoggedIn || !state.settings.enabled) { | ||
| return; | ||
| } | ||
|
|
||
| // Check inactivity every second | ||
| timeoutRef.current = setInterval(checkInactivity, 1000); | ||
| }, [isLoggedIn, state.settings.enabled, clearTimers, checkInactivity]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make checkInactivity read from refs to avoid stale closures
This removes state from the interval’s closure and keeps logic correct across updates.
- const checkInactivity = useCallback(() => {
- if (!isLoggedIn || !state.settings.enabled) {
+ const checkInactivity = useCallback(() => {
+ if (!isLoggedIn || !settingsRef.current.enabled) {
return;
}
-
- const now = Date.now();
- const timeSinceActivity = now - state.lastActivity;
- const inactiveSeconds = Math.floor(timeSinceActivity / 1000);
- const maxInactiveSeconds = state.settings.timeoutMinutes * 60;
- const warningThreshold = maxInactiveSeconds - state.settings.warningSeconds;
-
- if (inactiveSeconds >= maxInactiveSeconds) {
+ const now = Date.now();
+ const inactiveSeconds = Math.floor((now - lastActivityRef.current) / 1000);
+ const maxInactiveSeconds = settingsRef.current.timeoutMinutes * 60;
+ const warningThreshold = maxInactiveSeconds - settingsRef.current.warningSeconds;
+
+ if (inactiveSeconds >= maxInactiveSeconds) {
// Time to logout
handleLogout();
- } else if (inactiveSeconds >= warningThreshold && !state.isWarningVisible) {
+ } else if (inactiveSeconds >= warningThreshold && !isWarningVisibleRef.current) {
// Show warning
const remainingSeconds = maxInactiveSeconds - inactiveSeconds;
setState(prev => ({
...prev,
isWarningVisible: true,
isIdle: true,
remainingTime: remainingSeconds,
}));
onWarningShown?.();
- } else if (state.isWarningVisible) {
+ } else if (isWarningVisibleRef.current) {
// Update countdown
const remainingSeconds = maxInactiveSeconds - inactiveSeconds;
setState(prev => ({
...prev,
remainingTime: Math.max(0, remainingSeconds),
}));
}
- }, [isLoggedIn, state.settings, state.lastActivity, state.isWarningVisible, handleLogout, onWarningShown]);
+ }, [isLoggedIn, handleLogout, onWarningShown]);Also, startTimer should set the interval with the latest callback:
- timeoutRef.current = setInterval(checkInactivity, 1000);
+ intervalRef.current = setInterval(checkInactivity, 1000);Committable suggestion skipped: line range outside the PR's diff.
…meout, add new properties for user inactivity and browser close logout options, and implement related UI components for configuration.
…lement immediate logout on browser close using sendBeacon for reliable session termination.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs (1)
46-47: Wire up or remove SkipAppWarmUp (currently not bound to configuration)Bind this flag from IConfiguration so it actually influences warmup, or remove it.
public override void Initialize() { var thisAssembly = Assembly.GetExecutingAssembly(); + // Bind SkipAppWarmUp from configuration + var config = IocManager.Resolve<IConfiguration>(); + SkipAppWarmUp = config.GetValue<bool>("SkipAppWarmUp", false);shesha-reactjs/src/providers/autoLogout/provider.tsx (5)
56-63: Track volatile values in refs to avoid stale interval closuresStore
lastActivity,isWarningVisible, andsettingsin refs and keep them in sync.const [state, setState] = useState<IAutoLogoutState>(() => ({ @@ })); + const lastActivityRef = useRef<number>(Date.now()); + const isWarningVisibleRef = useRef<boolean>(false); + const settingsRef = useRef<IAutoLogoutSettings>(getEffectiveSettings()); + + useEffect(() => { + lastActivityRef.current = state.lastActivity; + isWarningVisibleRef.current = state.isWarningVisible; + settingsRef.current = state.settings; + }, [state.lastActivity, state.isWarningVisible, state.settings]);
95-111: Update the activity ref immediately on user activityEnsures the interval reads the latest value.
const recordActivity = useCallback(() => { const now = Date.now(); + lastActivityRef.current = now; setState(prev => {
113-147: Interval reads stale state; compute from refs insteadPrevents erroneous logouts and ignored resets.
- const checkInactivity = useCallback(() => { - if (!isLoggedIn || !state.settings.enabled) { + const checkInactivity = useCallback(() => { + if (!isLoggedIn || !settingsRef.current.enabled) { return; } - - const now = Date.now(); - const timeSinceActivity = now - state.lastActivity; - const inactiveSeconds = Math.floor(timeSinceActivity / 1000); - const timeoutSeconds = state.settings.timeoutMinutes * 60; - const maxInactiveSeconds = timeoutSeconds + state.settings.warningSeconds; - const warningThreshold = timeoutSeconds; // Show warning after timeout, not before + const now = Date.now(); + const inactiveSeconds = Math.floor((now - lastActivityRef.current) / 1000); + const timeoutSeconds = settingsRef.current.timeoutMinutes * 60; + // If you want warning BEFORE timeout, set threshold = Math.max(0, timeoutSeconds - settingsRef.current.warningSeconds) + const warningThreshold = timeoutSeconds; // current behavior: warning after timeout + const maxInactiveSeconds = timeoutSeconds + settingsRef.current.warningSeconds; @@ - } else if (inactiveSeconds >= warningThreshold && !state.isWarningVisible) { + } else if (inactiveSeconds >= warningThreshold && !isWarningVisibleRef.current) { @@ - } else if (state.isWarningVisible) { + } else if (isWarningVisibleRef.current) {
64-73: Clear the correct timer handleAdjust clear to the new
intervalRef.- const clearTimers = useCallback(() => { - if (timeoutRef.current) { - clearInterval(timeoutRef.current); - timeoutRef.current = undefined; - } - if (browserCloseTimeoutRef.current) { - clearTimeout(browserCloseTimeoutRef.current); - browserCloseTimeoutRef.current = undefined; - } - }, []); + const clearTimers = useCallback(() => { + if (intervalRef.current !== null) { + clearInterval(intervalRef.current); + intervalRef.current = null; + } + }, []);
148-158: Start interval with latest callback and handle re-starts safelyUse
intervalRefand avoid leaking old intervals.const startTimer = useCallback(() => { clearTimers(); @@ - // Check inactivity every second - timeoutRef.current = setInterval(checkInactivity, 1000); + intervalRef.current = setInterval(checkInactivity, 1000); }, [isLoggedIn, state.settings.enabled, clearTimers, checkInactivity]);Also consider restarting when timeout/warning values change (not only when
enabledchanges).
🧹 Nitpick comments (9)
shesha-core/src/Shesha.Framework/Configuration/Security/SecuritySettings.cs (1)
15-17: Doc clarity: specify 0 disables and unitsAdd that 0 disables auto logoff; units already clarified as minutes. Also consider inline validation to prevent negative values.
shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs (1)
134-137: Confirm semantics: “warning” occurs after timeout, not beforeFrontend provider currently shows the warning after
timeoutMinutes(grace window equalsLogoutWarningSeconds). If the intended UX is “warn before timeout”, adjust provider logic or rename setting/description to “grace period after inactivity threshold”.shesha-reactjs/src/components/idleTimerRenderer/index.tsx (1)
25-38: Convert to a true no-op wrapper to avoid dead state/effectsSince AutoLogoutProvider owns the logic, strip state and timers here to prevent confusion and future regressions.
-const autoLogoffTimeoutId: ISettingIdentifier = { name: 'Shesha.Security', module: 'Shesha' }; +const autoLogoffTimeoutId: ISettingIdentifier = { name: 'Shesha.Security', module: 'Shesha' }; // unused; can be removed @@ - const { styles } = useStyles(); - const { value: securitySettings } = useSettingValue(autoLogoffTimeoutId); - const timeoutValue = (securitySettings?.autoLogoffTimeout ?? 0) * 60; // Convert minutes to seconds - const warningSeconds = securitySettings?.logoutWarningSeconds ?? SIXTY; + // Deprecated: logic moved to AutoLogoutProvider @@ - const [state, setState] = useState<IIdleTimerState>(() => ({ - isIdle: false, - remainingTime: warningSeconds, - })); + // No internal state needed @@ - // Temporarily disabled - using AutoLogoutProvider instead - return <>{children}</>; + // Using AutoLogoutProvider instead + return <>{children}</>;Or delete this component and inline children at call sites.
Also applies to: 59-60
shesha-reactjs/src/components/settings/autoLogoutSettings.json (2)
35-36: Hidden expression allows fields when toggle is off; confirm intentWith
OR (AutoLogoffTimeout > 0), fields show even ifLogoutWhenUserInactiveis false. If the toggle should control visibility, prefer strict toggle gating.- "hidden": "!(data.LogoutWhenUserInactive === true || (data.AutoLogoffTimeout && data.AutoLogoffTimeout > 0))", + "hidden": "!(data.LogoutWhenUserInactive === true)"Also applies to: 48-49
65-66: Browser-close description mismatches implementationProvider attempts immediate logout on
beforeunload; it doesn’t “use the same timeout.” Rephrase to avoid confusion.- "description": "Automatically logout users when browser tab/window is closed or hidden. Uses the same timeout as inactive users.", + "description": "Automatically logs out when the browser tab/window is closed. Best-effort during unload; acts immediately (no timeout).",shesha-reactjs/src/providers/autoLogout/provider.tsx (4)
210-243: Avoid closure on state in activity handlerRead visibility flag from ref so activity during warning is correctly ignored/handled.
- if (state.isWarningVisible) { + if (isWarningVisibleRef.current) { return; }
245-297: Browser-close logout: verify endpoint and CSRF; remove unused timeout ref
sendBeacon('/api/services/app/Session/Logout', ...)may fail due to CORS/CSRF and missing headers. Treat as best-effort and ensure server endpoint accepts beacon POST without anti-forgery. Also,browserCloseTimeoutRefis removed per earlier diff.
- If unreliable, prefer clearing tokens/storage only; rely on short server-side session timeout.
- Confirm the correct logout URL for your backend and whether anonymous POST is allowed during unload.
299-309: AntD Progress status: ‘active’ isn’t valid for type="circle"Use ‘normal’ | ‘success’ | ‘exception’. Consider mapping >60 => 'normal', else 'exception'.
const getProgressStatus = () => { const percent = getProgressPercent(); - if (percent > 60) return 'normal'; - if (percent > 30) return 'active'; - return 'exception'; + return percent > 30 ? 'normal' : 'exception'; };
37-54: Enabled logic may ignore the toggle
enabledbecomes true ifautoLogoffTimeout > 0even whenlogoutWhenUserInactiveis false. If the toggle should control behavior, prioritize it.- enabled: securitySettings?.logoutWhenUserInactive || (securitySettings?.autoLogoffTimeout && securitySettings.autoLogoffTimeout > 0), + enabled: securitySettings?.logoutWhenUserInactive ?? ((securitySettings?.autoLogoffTimeout ?? 0) > 0),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
shesha-core/src/Shesha.Framework/Configuration/Security/SecuritySettings.cs(2 hunks)shesha-core/src/Shesha.Framework/SheshaFrameworkModule.cs(2 hunks)shesha-core/src/Shesha.Web.Host/appsettings.json(1 hunks)shesha-reactjs/src/components/idleTimerRenderer/index.tsx(2 hunks)shesha-reactjs/src/components/idleTimerRenderer/util.ts(1 hunks)shesha-reactjs/src/components/settings/autoLogoutSettings.json(1 hunks)shesha-reactjs/src/providers/autoLogout/models.ts(1 hunks)shesha-reactjs/src/providers/autoLogout/provider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shesha-reactjs/src/providers/autoLogout/models.ts
| /// <summary> | ||
| /// Enable logout when user is inactive | ||
| /// </summary> | ||
| public bool LogoutWhenUserInactive { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Warning time before logout in seconds | ||
| /// </summary> | ||
| public int LogoutWarningSeconds { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Enable logout when browser is closed/hidden | ||
| /// </summary> | ||
| public bool LogoutWhenBrowserClosed { get; set; } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation attributes for new settings to enforce safe ranges
Prevent misconfiguration and align with UI constraints.
using System.ComponentModel.DataAnnotations;
@@
- public bool LogoutWhenUserInactive { get; set; }
+ public bool LogoutWhenUserInactive { get; set; }
@@
- public int LogoutWarningSeconds { get; set; }
+ [Range(10, 300)]
+ public int LogoutWarningSeconds { get; set; }
@@
- public bool LogoutWhenBrowserClosed { get; set; }
+ public bool LogoutWhenBrowserClosed { get; set; }Optionally also validate:
- public int AutoLogoffTimeout { get; set; }
+ [Range(0, 1440)]
+ public int AutoLogoffTimeout { get; set; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Enable logout when user is inactive | |
| /// </summary> | |
| public bool LogoutWhenUserInactive { get; set; } | |
| /// <summary> | |
| /// Warning time before logout in seconds | |
| /// </summary> | |
| public int LogoutWarningSeconds { get; set; } | |
| /// <summary> | |
| /// Enable logout when browser is closed/hidden | |
| /// </summary> | |
| public bool LogoutWhenBrowserClosed { get; set; } | |
| /// <summary> | |
| /// Enable logout when user is inactive | |
| /// </summary> | |
| public bool LogoutWhenUserInactive { get; set; } | |
| /// <summary> | |
| /// Warning time before logout in seconds | |
| /// </summary> | |
| [Range(10, 300)] | |
| public int LogoutWarningSeconds { get; set; } | |
| /// <summary> | |
| /// Enable logout when browser is closed/hidden | |
| /// </summary> | |
| public bool LogoutWhenBrowserClosed { get; set; } |
🤖 Prompt for AI Agents
In shesha-core/src/Shesha.Framework/Configuration/Security/SecuritySettings.cs
around lines 60 to 74, the new settings lack validation attributes; add data
annotation attributes to enforce safe ranges: decorate LogoutWarningSeconds with
[Range(5, 3600)] (or another agreed minimum/maximum seconds) to prevent
too-small/too-large values, and optionally add [Required] if configuration must
provide the value; no validation attributes are needed for the boolean flags,
but you may add [Display(Name=...)] or [DefaultValue(false)] if desired for
clarity.
| export const getPercentage = (rt: number, maxTime: number = SIXTY) => (rt / maxTime) * 100; | ||
|
|
||
| export const getStatus = (rt: number): 'normal' | 'success' | 'exception' => | ||
| getPercentage(rt) >= 75 ? 'success' : getPercentage(rt) >= 45 ? 'normal' : 'exception'; | ||
| export const getStatus = (rt: number, maxTime: number = SIXTY): 'normal' | 'success' | 'exception' => | ||
| getPercentage(rt, maxTime) >= 75 ? 'success' : getPercentage(rt, maxTime) >= 45 ? 'normal' : 'exception'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against zero/negative maxTime to avoid NaN/Infinity
Add a floor to maxTime before division.
-export const getPercentage = (rt: number, maxTime: number = SIXTY) => (rt / maxTime) * 100;
+export const getPercentage = (rt: number, maxTime: number = SIXTY) => {
+ const denom = Math.max(1, maxTime | 0);
+ return (rt / denom) * 100;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPercentage = (rt: number, maxTime: number = SIXTY) => (rt / maxTime) * 100; | |
| export const getStatus = (rt: number): 'normal' | 'success' | 'exception' => | |
| getPercentage(rt) >= 75 ? 'success' : getPercentage(rt) >= 45 ? 'normal' : 'exception'; | |
| export const getStatus = (rt: number, maxTime: number = SIXTY): 'normal' | 'success' | 'exception' => | |
| getPercentage(rt, maxTime) >= 75 ? 'success' : getPercentage(rt, maxTime) >= 45 ? 'normal' : 'exception'; | |
| export const getPercentage = (rt: number, maxTime: number = SIXTY) => { | |
| const denom = Math.max(1, maxTime | 0); | |
| return (rt / denom) * 100; | |
| }; |
🤖 Prompt for AI Agents
In shesha-reactjs/src/components/idleTimerRenderer/util.ts around lines 5 to 8,
the functions divide by maxTime without guarding against zero or negative values
which can produce NaN/Infinity; clamp maxTime to a safe minimum (e.g. const
safeMax = Math.max(1, maxTime)) before doing the division and use that safeMax
in getPercentage, and update getStatus to compute percentage once using the
clamped value (avoid calling getPercentage twice) so the status logic uses the
safe percentage.
| const timeoutRef = useRef<NodeJS.Timeout>(); | ||
| const browserCloseTimeoutRef = useRef<NodeJS.Timeout>(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use DOM-safe timer types and a single intervalRef; remove unused browserCloseTimeoutRef
NodeJS.Timeout is incorrect in browser builds; also browserCloseTimeoutRef is never set.
- const timeoutRef = useRef<NodeJS.Timeout>();
- const browserCloseTimeoutRef = useRef<NodeJS.Timeout>();
+ const intervalRef = useRef<ReturnType<typeof setInterval> | null>(null);And update all usages accordingly (see diffs below).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/autoLogout/provider.tsx around lines 34 to 36,
replace NodeJS-specific timer types and the unused browserCloseTimeoutRef with a
single DOM-safe timer ref: remove browserCloseTimeoutRef, rename timeoutRef to
something like timeoutIdRef and type it as ReturnType<typeof setTimeout> | null
(or number | null), initialize with useRef<...>(null), and update all places
that set/clear timeouts to use window.clearTimeout/clearInterval with that ref
and handle null checks; ensure any interval ref follows the same DOM-safe typing
and that unused variable declarations are removed.
|
implementation complete.. the merge conflicts will be dealt with |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
shesha-core/src/Shesha.Application/Shesha.Application.csproj (1)
137-137: Reduce maintenance by globbing all .shaconfig files instead of per-file entriesManually tracking every migration file is error-prone and churn-heavy. Consider globbing:
Apply this refactor (illustrative; removes the long per-file lists):
- <ItemGroup> - <!-- many explicit None Remove entries --> - <None Remove="ConfigMigrations\package20250910_1513.shaconfig" /> - </ItemGroup> + <ItemGroup> + <None Remove="ConfigMigrations\*.shaconfig" /> + </ItemGroup> - <ItemGroup> - <!-- many explicit EmbeddedResource Include entries --> - <EmbeddedResource Include="ConfigMigrations\package20250910_1513.shaconfig" /> - </ItemGroup> + <ItemGroup> + <EmbeddedResource Include="ConfigMigrations\*.shaconfig" /> + </ItemGroup>If you need to control inclusion, you can use Exclude or a property gate, e.g.:
- Exclude="ConfigMigrations\experimental*.shaconfig"
- Condition="'$(EmbedAllConfigMigrations)' == 'true'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-core/src/Shesha.Application/Shesha.Application.csproj(2 hunks)
🔇 Additional comments (1)
shesha-core/src/Shesha.Application/Shesha.Application.csproj (1)
89-89: Ignore .shaconfig embedding review – no matching files found
The repository contains no Shesha.Application.csproj and no *.shaconfig under ConfigMigrations; the original comment does not apply.Likely an incorrect or invalid review comment.
2 participants
simple implementation
Summary by CodeRabbit
New Features
Improvements
Bug Fixes