Skip to content

feat(dashboard): auto-save config with 500ms debounce#199

Merged
BillChirico merged 5 commits intomainfrom
feat/issue-189
Mar 2, 2026
Merged

feat(dashboard): auto-save config with 500ms debounce#199
BillChirico merged 5 commits intomainfrom
feat/issue-189

Conversation

@BillChirico
Copy link
Collaborator

Summary

Implements auto-save for the config dashboard. No more manual Save button.

Changes

  • Auto-save — config changes are saved automatically 500ms after the last edit (debounced). Zero changes → zero network calls.
  • Status indicator — replaces the Save button with a compact inline status:
    • (idle: nothing shown)
    • Saving... with spinner while in flight
    • ✓ Saved on success
    • ⚠ Save failed + Retry on error
  • Initial load guardisLoadingConfigRef prevents the config load from triggering a spurious auto-save
  • Ctrl+S — still works; clears the debounce timer and saves immediately
  • Validation errors — auto-save is suppressed while there are validation errors; a destructive banner replaces the old yellow banner
  • Error recoveryRetry button re-fires saveChanges inline; partial failures still merge succeeded sections

Tests

4 new tests in config-editor-autosave.test.tsx:

  • No PATCH on initial config load
  • Validation banner shown + no PATCH when system prompt is too long
  • Saving... spinner while PATCH is in-flight
  • Save failed + Retry button on PATCH 500

Test Results

Test Files  21 passed (21)
     Tests  145 passed (145)

Closes #189

- Remove 'Save Changes' button; saving now fires automatically 500ms
  after the last config change (no changes → no network call)
- Add saveStatus state ('idle' | 'saving' | 'saved' | 'error') with
  AutoSaveStatus component showing spinner, check, or error+retry
- Add isLoadingConfigRef guard so the initial fetchConfig load never
  triggers a spurious PATCH
- Ctrl+S still works: clears debounce timer and saves immediately
- Keep 'beforeunload' warning for validation errors that block save
- Replace yellow unsaved-changes banner with a destructive validation
  error banner (only shown when save is actually blocked)
- Error state shows 'Save failed' + 'Retry' button for user recovery

Closes #189
- No PATCH on initial config load
- Validation error banner suppresses auto-save
- 'Saving...' spinner visible while PATCH in-flight
- 'Save failed' + Retry button on PATCH error
Copilot AI review requested due to automatic review settings March 2, 2026 04:10
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 2, 2026

Warning

Rate limit exceeded

@BillChirico has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2791afb and 61d6f1d.

📒 Files selected for processing (2)
  • web/src/components/dashboard/config-editor.tsx
  • web/tests/components/dashboard/config-editor-autosave.test.tsx
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/issue-189

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements debounced auto-save for the dashboard config editor, replacing the manual “Save Changes” button with an inline save-status indicator and adding tests to validate the behavior.

Changes:

  • Adds 500ms debounced auto-save (plus Ctrl/Cmd+S immediate save) and suppresses auto-save during validation errors / initial load.
  • Introduces an inline auto-save status indicator (Saving/Saved/Error + Retry).
  • Adds a new test suite covering initial-load no-PATCH, validation suppression, saving UI, and error UI.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File Description
web/src/components/dashboard/config-editor.tsx Adds auto-save debounce logic, status indicator UI, and wiring into existing save/load flows.
web/tests/components/dashboard/config-editor-autosave.test.tsx Adds integration-style tests for auto-save behavior and status UI states.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link

greptile-apps bot commented Mar 2, 2026

Greptile Summary

Replaces manual save button with auto-save that debounces changes at 500ms. Config changes trigger automatic persistence with inline status indicators (Saving.../Saved/Save failed + Retry). Includes initial load guard to prevent spurious saves, validation error blocking, and Ctrl+S for immediate save. Tests verify no PATCH on mount, validation blocking, and all status states.

Key improvements:

  • Auto-save eliminates need for manual save button, improving UX
  • Debouncing prevents excessive network calls
  • Status indicators provide clear feedback
  • Comprehensive test coverage for core scenarios

Issues found:

  • Ref-based change detection has a timing race: if server responds before React effect runs, pending user edits can be overwritten (low probability but possible on fast networks)
  • Discard button doesn't clear save status indicator

Confidence Score: 3/5

  • This PR improves UX but has a low-probability race condition that could lose user edits
  • Score reflects solid implementation with good test coverage, but the ref timing issue in the race condition fix (line 248-251, 389) is a real logic bug that could lose user data under fast network conditions. The mitigation attempt doesn't fully resolve the original race condition comment. The missing status reset on discard is minor. Auto-save logic, debouncing, and validation blocking are correct.
  • Pay close attention to web/src/components/dashboard/config-editor.tsx lines 248-251 and 389 for the ref timing race condition

Important Files Changed

Filename Overview
web/src/components/dashboard/config-editor.tsx Implements auto-save with 500ms debounce, status indicator, and race condition mitigation. Has a timing issue in ref-based change detection that could lose edits on fast servers, plus missing status reset on discard.
web/tests/components/dashboard/config-editor-autosave.test.tsx Comprehensive test coverage for auto-save: validates no spurious saves on load, validation error blocking, status UI states, and error retry.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    Start([User edits config]) --> Check{Check guards}
    Check -->|isLoading=true| Block[Block auto-save]
    Check -->|No changes| Block
    Check -->|Validation errors| Block
    Check -->|Already saving| Block
    Check -->|All guards pass| ClearTimer[Clear existing debounce timer]
    ClearTimer --> SetTimer[Set 500ms debounce timer]
    SetTimer --> Wait[Wait 500ms]
    Wait --> Save[Execute saveChanges]
    Save --> SetSaving[Set status='saving']
    SetSaving --> Snapshot[Capture draftSnapshot]
    Snapshot --> Patch[Send PATCH requests]
    Patch --> Success{All succeeded?}
    Success -->|Yes| CheckChanges{draftConfigRef == snapshot?}
    CheckChanges -->|Equal - no edits during save| Reload[fetchConfig from server]
    CheckChanges -->|Not equal - user edited| UpdateSaved[Update savedConfig locally]
    UpdateSaved --> StatusSaved[Set status='saved']
    Reload --> StatusSaved
    StatusSaved --> TriggerNext{hasChanges?}
    TriggerNext -->|Yes| Start
    TriggerNext -->|No| Idle[Set status='idle']
    Success -->|Partial/failed| Error[Set status='error']
    Error --> ShowRetry[Show Retry button]
    Block --> End([End])
    Idle --> End
    ShowRetry --> End
Loading

Last reviewed commit: 61d6f1d

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

… successful save

Add skipSaveStatusReset parameter to fetchConfig so that post-save reloads
preserve the 'saved' status indicator instead of immediately resetting to 'idle'.
…ns, add idle/saved coverage

- Replace real setTimeout delays with vi.useFakeTimers() + vi.advanceTimersByTimeAsync()
  for deterministic, fast debounce tests
- Add afterEach cleanup: vi.unstubAllGlobals() + vi.useRealTimers()
- Replace toBeTruthy() with toBeInTheDocument() for Testing Library queries
- Add idle state test (no status indicator shown after load)
- Add saved state test (shows 'Saved' after successful save)
- Update file-level comment to list all four states
Copilot AI review requested due to automatic review settings March 2, 2026 11:36
@BillChirico BillChirico merged commit a710098 into main Mar 2, 2026
10 of 12 checks passed
@BillChirico BillChirico deleted the feat/issue-189 branch March 2, 2026 11:37
BillChirico added a commit that referenced this pull request Mar 2, 2026
* feat(dashboard): replace manual save with auto-save (500ms debounce)

- Remove 'Save Changes' button; saving now fires automatically 500ms
  after the last config change (no changes → no network call)
- Add saveStatus state ('idle' | 'saving' | 'saved' | 'error') with
  AutoSaveStatus component showing spinner, check, or error+retry
- Add isLoadingConfigRef guard so the initial fetchConfig load never
  triggers a spurious PATCH
- Ctrl+S still works: clears debounce timer and saves immediately
- Keep 'beforeunload' warning for validation errors that block save
- Replace yellow unsaved-changes banner with a destructive validation
  error banner (only shown when save is actually blocked)
- Error state shows 'Save failed' + 'Retry' button for user recovery

Closes #189

* test(dashboard): add auto-save tests for ConfigEditor

- No PATCH on initial config load
- Validation error banner suppresses auto-save
- 'Saving...' spinner visible while PATCH in-flight
- 'Save failed' + Retry button on PATCH error

* fix(dashboard): prevent fetchConfig from overwriting saveStatus after successful save

Add skipSaveStatusReset parameter to fetchConfig so that post-save reloads
preserve the 'saved' status indicator instead of immediately resetting to 'idle'.

* test(dashboard): use fake timers, restore vi.stubGlobal, fix assertions, add idle/saved coverage

- Replace real setTimeout delays with vi.useFakeTimers() + vi.advanceTimersByTimeAsync()
  for deterministic, fast debounce tests
- Add afterEach cleanup: vi.unstubAllGlobals() + vi.useRealTimers()
- Replace toBeTruthy() with toBeInTheDocument() for Testing Library queries
- Add idle state test (no status indicator shown after load)
- Add saved state test (shows 'Saved' after successful save)
- Update file-level comment to list all four states

---------

Co-authored-by: Bill Chirico <bill@volvox.gg>
BillChirico added a commit that referenced this pull request Mar 2, 2026
* security: escape user content in triage prompt delimiters (#164)

Add escapePromptDelimiters() to HTML-encode < and > in user-supplied
message content before it is inserted between XML-style section tags
in the LLM prompt.

Without escaping, a crafted message containing the literal text
`</messages-to-evaluate>` could break out of the user-content section
and inject attacker-controlled instructions into the prompt structure.

Changes:
- Add escapePromptDelimiters(text) utility exported from triage-prompt.js
- Apply escape to m.content and m.replyTo.content in buildConversationText()
- Add 13 new tests covering the escape function and injection scenarios

Closes #164

* security: escape & chars and author fields in prompt delimiters

* fix(security): escape & in prompt delimiters and escape author fields

- Add & → &amp; escape first in escapePromptDelimiters() to prevent
  HTML entity bypass attacks (e.g. &lt;/messages-to-evaluate&gt;)
- Also escape m.author and m.replyTo.author since Discord display
  names are user-controlled and can contain < / > characters

Addresses review feedback on PR #204.

* fix: guard replyTo.content before .slice() to handle null/undefined

* perf: SQL-based conversation pagination + missing DB indexes (#221)

Fixes three performance bottlenecks identified in code review of
recently merged features (PR #121 conversations viewer, PR #190 AI feedback).

## Changes

### migrations/004_performance_indexes.cjs (new)
Four new indexes targeting hot query paths:

- idx_ai_feedback_guild_created (guild_id, created_at DESC)
  getFeedbackTrend() and getRecentFeedback() filtered by guild_id
  AND created_at but only had a single-column guild_id index, forcing
  a full guild scan + sort on every trend/recent call.

- idx_conversations_content_trgm (GIN, pg_trgm)
  content ILIKE '%...%' search was a sequential scan. GIN/trgm index
  reduces this from O(n) to O(log n * trigram matches).
  Requires pg_trgm extension (added idempotently).

- idx_conversations_guild_created (guild_id, created_at DESC)
  Default 30-day listing query filters guild_id + created_at. The
  existing 3-column (guild_id, channel_id, created_at) composite is
  suboptimal when channel_id is not in the predicate.

- idx_flagged_messages_guild_message (guild_id, message_id)
  Conversation detail + flag endpoints query flagged_messages by
  guild_id AND message_id = ANY(...). Existing index only covers
  (guild_id, status).

### src/api/routes/conversations.js
**GET / — Replace in-memory pagination with SQL CTE grouping**

Before: fetched up to 10,000 message rows into Node memory, grouped
them in JavaScript (O(n) time + memory), then sliced for pagination.
Every page request loaded the full 10k row dataset.

After: single SQL query using window functions (LAG + SUM OVER) to
identify conversation boundaries and aggregate summaries directly.
COUNT(*) OVER() provides total count without a second query.
Pagination happens at the DB with LIMIT/OFFSET on summary rows.
Memory overhead is now proportional to page size (default 25), not
total conversation volume.

Removed now-unused buildConversationSummary() helper (logic inlined
into the SQL-side aggregation).

**POST /:conversationId/flag — Parallel verification queries**

Before: msgCheck and anchorCheck ran sequentially (~2× RTT).
After: both run in parallel via Promise.all (1× RTT for verification).

### tests/api/routes/conversations.test.js
Updated 'should return paginated conversations' test to mock the new
SQL CTE response shape (pre-aggregated summary rows) instead of raw
message rows. All 41 conversation tests pass.

* feat: channel-level quiet mode via bot mention (#173) (#213)

* feat: quiet mode per-channel via bot mention (#173)

- Add quietMode.js module with Redis+memory storage
- Parse duration from natural language (30m, 1 hour, etc.)
- Permission gated via config.quietMode.allowedRoles
- Commands: quiet, unquiet, status
- Suppress AI responses during quiet mode in events.js
- Add quietMode section to config.json (disabled by default)
- Add quietMode to configAllowlist.js for dashboard editing

* test: add quiet mode tests (41 tests, all passing)

* style: fix biome formatting in quietMode.js, events.js, and test

* fix(web): fix ai-feedback-stats TypeScript and formatting errors

* fix: gate quiet mode checks on enabled flag, validate TTL, honor maxDurationMinutes config

- events.js: Wrap isQuietMode() calls in guildConfig.quietMode?.enabled check
  to avoid unnecessary Redis lookups and prevent stale records from suppressing
  AI responses when the feature is disabled (PRRT_kwDORICdSM5xdbmp, PRRT_kwDORICdSM5xdbmx)

- quietMode.js: Add TTL validation in setQuiet() to guard against 0, negative,
  or NaN values that would error in Redis (PRRT_kwDORICdSM5xdbm3)

- quietMode.js: Update parseDurationFromContent() to accept config parameter
  and honor guildConfig.quietMode.maxDurationMinutes. Also clamp defaultSeconds
  to the effective max (PRRT_kwDORICdSM5xdbm_)

- configValidation.js: Add quietMode schema entry with enabled, maxDurationMinutes,
  and allowedRoles properties (PRRT_kwDORICdSM5xdbnH)

* style: fix biome formatting in quietMode.js and ai-feedback-stats.tsx

* feat: audit log improvements — CSV/JSON export and real-time WebSocket stream (#215)

* feat: audit log improvements — CSV/JSON export, real-time WebSocket stream

- Add GET /:id/audit-log/export endpoint (CSV and JSON, up to 10k rows)
- Add /ws/audit-log WebSocket server for real-time audit entry broadcast
- Refactor buildFilters() shared helper to eliminate duplication
- Hook broadcastAuditEntry() into insertAuditEntry (RETURNING id+created_at)
- Wire setupAuditStream/stopAuditStream into startServer/stopServer lifecycle
- Add escapeCsvValue/rowsToCsv helpers with full test coverage
- 30 route tests + 17 WebSocket stream tests, all green

Closes #136

* fix: PR #215 review feedback - audit stream fixes

- ws.ping() crash: guard with readyState check + try/catch to avoid
  crashing heartbeat interval when socket not OPEN
- stopAuditStream race: make setupAuditStream async and await
  stopAuditStream() to prevent concurrent WebSocketServer creation
- Query param array coercion: add typeof === 'string' checks for
  startDate/endDate to handle Express string|string[]|undefined
- CSV CRLF quoting: add \r to RFC 4180 special-char check for proper
  Windows line ending handling
- Test timeouts: make AUTH_TIMEOUT_MS configurable via
  AUDIT_STREAM_AUTH_TIMEOUT_MS env var, use 100ms in tests

* feat: voice channel activity tracking — join/leave/move, leaderboard, export (#212)

* feat: add voice_sessions migration (#135)

* feat: add voice tracking module — join/leave/move/flush/leaderboard (#135)

* feat: wire voiceStateUpdate handler into event registration (#135)

* feat: add /voice command — leaderboard, stats, export subcommands (#135)

* feat: add voice config defaults to config.json (#135)

* feat: wire voice flush start/stop into bot lifecycle (#135)

* feat: add voice to config API allowlist (#135)

* fix: SQL UPDATE subquery for closeSession, fix import order (#135)

* fix(voice): resolve race conditions and missing config schema

- Fix openSession: update in-memory state only AFTER DB INSERT succeeds
- Fix closeSession: delete from in-memory state only AFTER DB UPDATE succeeds
- Fix: allow closeSession on leave/move even when feature is disabled
- Fix migration: add UNIQUE constraint to partial index to prevent duplicates
- Fix: move 'Voice join' log to after openSession succeeds
- Add voice config to CONFIG_SCHEMA for validation

---------

Co-authored-by: Bill <bill@example.com>

* feat(dashboard): auto-save config with 500ms debounce (#199)

* feat(dashboard): replace manual save with auto-save (500ms debounce)

- Remove 'Save Changes' button; saving now fires automatically 500ms
  after the last config change (no changes → no network call)
- Add saveStatus state ('idle' | 'saving' | 'saved' | 'error') with
  AutoSaveStatus component showing spinner, check, or error+retry
- Add isLoadingConfigRef guard so the initial fetchConfig load never
  triggers a spurious PATCH
- Ctrl+S still works: clears debounce timer and saves immediately
- Keep 'beforeunload' warning for validation errors that block save
- Replace yellow unsaved-changes banner with a destructive validation
  error banner (only shown when save is actually blocked)
- Error state shows 'Save failed' + 'Retry' button for user recovery

Closes #189

* test(dashboard): add auto-save tests for ConfigEditor

- No PATCH on initial config load
- Validation error banner suppresses auto-save
- 'Saving...' spinner visible while PATCH in-flight
- 'Save failed' + Retry button on PATCH error

* fix(dashboard): prevent fetchConfig from overwriting saveStatus after successful save

Add skipSaveStatusReset parameter to fetchConfig so that post-save reloads
preserve the 'saved' status indicator instead of immediately resetting to 'idle'.

* test(dashboard): use fake timers, restore vi.stubGlobal, fix assertions, add idle/saved coverage

- Replace real setTimeout delays with vi.useFakeTimers() + vi.advanceTimersByTimeAsync()
  for deterministic, fast debounce tests
- Add afterEach cleanup: vi.unstubAllGlobals() + vi.useRealTimers()
- Replace toBeTruthy() with toBeInTheDocument() for Testing Library queries
- Add idle state test (no status indicator shown after load)
- Add saved state test (shows 'Saved' after successful save)
- Update file-level comment to list all four states

---------

Co-authored-by: Bill Chirico <bill@volvox.gg>

* feat: Reaction role menus (#162) (#205)

* feat: reaction role menus - core module, command, event hooks, migration

Implements issue #162: reaction role menus.

- Add migration 004 creating reaction_role_menus and reaction_role_entries tables
- Add src/modules/reactionRoles.js with DB helpers, embed builder, event handlers
- Add src/commands/reactionrole.js with /reactionrole create|add|remove|delete|list
- Wire handleReactionRoleAdd/Remove into registerReactionHandlers in events.js

Roles are granted on reaction add and revoked on reaction remove.
All mappings persist in PostgreSQL across bot restarts.

* test: reaction role menus - 40 tests covering module and command

- tests/modules/reactionRoles.test.js: resolveEmojiString, buildReactionRoleEmbed,
  all DB helpers, handleReactionRoleAdd, handleReactionRoleRemove
- tests/commands/reactionrole.test.js: all 5 subcommands (create, add, remove,
  delete, list) including error paths and guild ownership checks
- Fix biome lint: import sort order + unused import removal

* fix: remove unused import in reactionrole command

---------

Co-authored-by: Bill Chirico <bill@volvox.gg>

* fix(security): validate GitHub owner/repo format before gh CLI call (#198)

* fix(security): validate GitHub owner/repo format before gh CLI call

Prevents API path traversal by validating owner/repo segments against
a strict allowlist regex before interpolating them into the gh CLI
invocation.

Adds:
- VALID_GH_NAME regex (/^[a-zA-Z0-9._-]+$/)
- isValidGhRepo() helper (exported for testing)
- Guard in fetchRepoEvents() — returns [] and warns on invalid input
- Strengthened guard in pollGuildFeed() split logic

Fixes #160

* test(security): add validation tests for GitHub owner/repo format

Covers isValidGhRepo(), VALID_GH_NAME regex, and fetchRepoEvents()
validation guard introduced in fix for #160.

19 new tests verify:
- Valid alphanumeric/dot/hyphen/underscore names pass
- Path traversal (../../etc/passwd) is rejected at both entry points
- Slashes, empty strings, non-strings, spaces all rejected
- Shell metacharacters (; && $()) blocked
- gh CLI is NOT invoked when validation fails
- warn() fires with the invalid values (observable audit trail)
- Valid owner/repo still reach gh CLI unchanged

* fix(security): reject pure-dot owner/repo names to prevent path traversal

* test(githubFeed): add tests for pure-dot path traversal bypass

---------

Co-authored-by: Bill Chirico <bill@volvox.gg>

---------

Co-authored-by: Bill <bill@example.com>
Co-authored-by: Bill Chirico <bill@volvox.gg>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +408 to +419
useEffect(() => {
// Don't auto-save during initial config load or if no changes
if (isLoadingConfigRef.current || !hasChanges || hasValidationErrors || saving) return;

// Clear any pending timer
if (autoSaveTimerRef.current) {
clearTimeout(autoSaveTimerRef.current);
}

autoSaveTimerRef.current = setTimeout(() => {
void saveChanges();
}, 500);
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When saveStatus is saved, making a new edit doesn’t clear it, so the UI can still display “Saved” during the 500ms debounce window even though there are unsaved changes. Consider resetting saveStatus back to idle (or a dedicated “pending changes” state) as soon as hasChanges becomes true, so the indicator doesn’t misrepresent the current persistence state.

Copilot uses AI. Check for mistakes.
Comment on lines +226 to +234
// Use a macrotask so the state setter for draftConfig fires before we clear the flag
setTimeout(() => {
isLoadingConfigRef.current = false;
}, 0);
setDmStepsRaw((data.welcome?.dmSequence?.steps ?? []).join('\n'));
setProtectRoleIdsRaw((data.moderation?.protectRoles?.roleIds ?? []).join(', '));
} catch (err) {
if ((err as Error).name === 'AbortError') return;
isLoadingConfigRef.current = false;
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLoadingConfigRef is cleared via setTimeout(..., 0) without checking that the completing request is still the latest one. If the guild changes quickly (or multiple fetchConfig calls overlap), an earlier request can flip the ref to false while a newer load is still in progress, allowing auto-save to run during load. Consider tracking a request id / comparing abortRef.current === controller inside the timeout (and in the error/abort paths) before clearing the flag.

Suggested change
// Use a macrotask so the state setter for draftConfig fires before we clear the flag
setTimeout(() => {
isLoadingConfigRef.current = false;
}, 0);
setDmStepsRaw((data.welcome?.dmSequence?.steps ?? []).join('\n'));
setProtectRoleIdsRaw((data.moderation?.protectRoles?.roleIds ?? []).join(', '));
} catch (err) {
if ((err as Error).name === 'AbortError') return;
isLoadingConfigRef.current = false;
// Use a macrotask so the state setter for draftConfig fires before we clear the flag.
// Only clear the loading flag if this request is still the active one.
setTimeout(() => {
if (abortRef.current === controller) {
isLoadingConfigRef.current = false;
}
}, 0);
setDmStepsRaw((data.welcome?.dmSequence?.steps ?? []).join('\n'));
setProtectRoleIdsRaw((data.moderation?.protectRoles?.roleIds ?? []).join(', '));
} catch (err) {
if ((err as Error).name === 'AbortError') return;
// Only clear the loading flag if this request is still the active one.
if (abortRef.current === controller) {
isLoadingConfigRef.current = false;
}

Copilot uses AI. Check for mistakes.
Comment on lines +387 to +391
setSaveStatus('saved');
// Full success: check if draft changed during save (user edited while saving)
if (deepEqual(draftConfigRef.current, draftSnapshot)) {
// No changes during save — safe to reload authoritative version
await fetchConfig(guildId, { skipSaveStatusReset: true });
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On full success, this sets status to saved and then calls fetchConfig, which toggles the global loading state and re-fetches the entire config. With auto-save, this can introduce frequent extra GETs and briefly replace the editor with the loading screen after each save. Consider avoiding the full reload (update savedConfig from draftSnapshot / server response) or adding a background-refresh mode that doesn’t flip loading/hide the editor.

Copilot uses AI. Check for mistakes.
<div className="flex items-center gap-2">
<div className="flex items-center gap-3">
{/* Auto-save status indicator */}
<AutoSaveStatus status={saveStatus} onRetry={saveChanges} />
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoSaveStatus is rendered solely from saveStatus, so it can remain visible (e.g., Save failed or Saved) even after the user discards changes and hasChanges becomes false. Consider resetting saveStatus to idle when discarding/when hasChanges becomes false, and wrap onRetry to also clear any pending debounce timer (similar to the Ctrl+S handler) to avoid double-saves.

Suggested change
<AutoSaveStatus status={saveStatus} onRetry={saveChanges} />
<AutoSaveStatus status={hasChanges ? saveStatus : 'idle'} onRetry={saveChanges} />

Copilot uses AI. Check for mistakes.
Comment on lines +2326 to +2333
<output className="flex items-center gap-1.5 text-sm text-destructive">
<AlertCircle className="h-4 w-4" aria-hidden="true" />
Save failed
<button
type="button"
onClick={onRetry}
className="ml-1 inline-flex items-center gap-1 rounded px-1.5 py-0.5 text-xs font-medium underline-offset-2 hover:underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
aria-label="Retry save"
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoSaveStatus uses <output> for the status text, but it isn’t marked as a live region, and the error variant nests an interactive <button> inside <output>. For better screen reader behavior, consider using role="status"/aria-live="polite" on a non-output wrapper (e.g., a <div>/<span>) and keep the Retry button as a sibling element.

Copilot uses AI. Check for mistakes.
Comment on lines +144 to +146
// Type more than SYSTEM_PROMPT_MAX_LENGTH chars (20000)
const tooLong = 'x'.repeat(20001);
await act(async () => {
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test hard-codes the system prompt max length as 20000, but the app constant SYSTEM_PROMPT_MAX_LENGTH is currently 4000 (web/src/types/config.ts). Import and use the shared constant here (and build the over-limit string based on it) to avoid drift and to keep the test input smaller/faster.

Copilot uses AI. Check for mistakes.
Comment on lines +248 to +251
// Keep draftConfigRef in sync for race-condition detection in saveChanges
useEffect(() => {
draftConfigRef.current = draftConfig;
}, [draftConfig]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref update timing issue: draftConfigRef.current is updated asynchronously in this effect after renders, but saveChanges (line 389) checks it synchronously after PATCH completes. If the server responds before this effect runs (e.g., user types → render scheduled → PATCH completes → ref still has old value), the check incorrectly concludes no changes occurred, calls fetchConfig, and overwrites the user's pending edits.

Consider updating the ref synchronously when setDraftConfig is called, or use a different approach like comparing against the latest state in a ref callback.

Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/dashboard/config-editor.tsx
Line: 248-251

Comment:
ref update timing issue: `draftConfigRef.current` is updated asynchronously in this effect after renders, but `saveChanges` (line 389) checks it synchronously after PATCH completes. If the server responds before this effect runs (e.g., user types → render scheduled → PATCH completes → ref still has old value), the check incorrectly concludes no changes occurred, calls `fetchConfig`, and overwrites the user's pending edits.

Consider updating the ref synchronously when `setDraftConfig` is called, or use a different approach like comparing against the latest state in a ref callback.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: automatic config saving in web dashboard

2 participants