Feature: recurring tasks #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add RecurrencePattern entity with Fixed (calendar-based) and Relative (completion-based) recurrence types - Support weekly patterns with specific days, monthly with day-of-month or Nth weekday, yearly patterns - Implement natural language parser for recurrence input (e.g., "every Monday", "2 weeks after completion") - Client-side instance spawning on task completion for offline support - Add recurrence UI to EditActionModal and ProcessPage - Include PowerSync sync rules and connector support for RecurrencePatterns - Add RecurrenceService with full CRUD and sync endpoints - Add unit tests for RecurrenceService Known issues to fix in follow-up: - First action instance not created when pattern is created - Parser tests not yet added Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Race condition prevention:
- Add check for existing active action before spawning duplicate in completion flow
Frontend improvements:
- Add RecurrenceParser tests (47 tests) for natural language parsing
- Extract RecurrenceField component from EditActionModal and ProcessPage
- Improve sync error message with table, ID, and debugging hint
- Fix schema.config.js customSelect to store clean SQL (prefix added by generator)
Backend fixes:
- Add missing EnergyLevel import in RecurrencePatternDto
- Standardize logging to use {Id} consistently in RecurrenceService
Test fixes:
- Fix FK constraint issues in RecurrenceServiceTests by using null for CurrentActionItemId
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds full recurring-task support across backend and frontend: new RecurrencePattern domain model, DB migration, service and API endpoints (including sync paths), frontend parsing/UI/hooks/components, PowerSync integration updates, Electron OAuth hardening and guards, sync rule/schema updates, docs, tests, and styling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as React UI
participant LocalDB as Local DB / Queue
participant Connector as PowerSync Connector
participant API as GTD API
participant Cloud as Cloud DB
User->>UI: Create recurring item + pattern
UI->>UI: parseRecurrence / build pattern
UI->>LocalDB: INSERT RecurrencePattern + ActionItem (queue)
LocalDB->>Connector: upload queued records
Connector->>Connector: getAuthToken / transform enums & dates
Connector->>API: POST /api/recurrence (Create)
API->>Cloud: persist RecurrencePattern
API-->>Connector: return created ids
Connector->>LocalDB: update local rows with server ids
Note over User,UI: On completion of recurring action
User->>UI: mark ActionItem completed
UI->>LocalDB: UPDATE ActionItem (completed)
UI->>LocalDB: READ RecurrencePattern
UI->>UI: calculateNextOccurrence()
UI->>LocalDB: INSERT new ActionItem (next occurrence) and UPDATE RecurrencePattern
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/scripts/generate-sync-rules.js (1)
210-232: Multi-linecustomSelectcan generate invalid YAML.Only the first line gets YAML indentation; subsequent lines from a multi‑line
customSelectcan start at column 0, breaking the list item. Normalize indentation for all lines before emission.✅ Suggested fix (indent all select lines)
function generateSyncRulesYaml() { const selectStatements = []; + const indent = ' '; for (const [entityName, config] of Object.entries(powerSyncConfig.entities)) { const schema = schemas[entityName]; @@ - const selectSql = config.customSelect + const selectSql = config.customSelect ? config.customSelect.trim() : generateSelect(config.tableName, extractFields(schema, config)); // Ensure proper YAML list format with indentation - const select = selectSql.startsWith('- ') ? selectSql : `- ${selectSql}`; + const selectLine = selectSql.startsWith('- ') ? selectSql : `- ${selectSql}`; + const select = selectLine.replace(/\n/g, `\n${indent}`); selectStatements.push({ entityName, select }); } const dataSection = selectStatements - .map(({ entityName, select }) => ` # ${entityName}\n ${select}`) + .map(({ entityName, select }) => `${indent}# ${entityName}\n${indent}${select}`) .join('\n\n');frontend/src/lib/powersync/connector.ts (1)
183-204: Guard PATCH against partial opData fallback.When the local row is missing,
getFullRecordfalls back toopData(line 239). However, PowerSync PATCH operations only contain changed fields (id + deltas), not the full record. Using incomplete opData as a fallback for PATCH will send a partial record to the backend, causing unintended field deletions. PUT operations can safely use the opData fallback since PowerSync PUT contains the full row.Add an
allowPartialoption togetFullRecord: allow fallback for PUT/new records, reject or fail fast for PATCH when the local row is missing.Suggested implementation
- const fullRecord = await this.getFullRecord(database, table, entry.id, entry.opData) + const fullRecord = await this.getFullRecord(database, table, entry.id, entry.opData, { allowPartial: true }) ... - const fullRecord = await this.getFullRecord(database, table, entry.id, entry.opData) + const fullRecord = await this.getFullRecord(database, table, entry.id, entry.opData, { allowPartial: false }) ... - private async getFullRecord( + private async getFullRecord( database: AbstractPowerSyncDatabase, table: string, id: string, - opData?: Record<string, unknown> + opData?: Record<string, unknown>, + options?: { allowPartial?: boolean } ): Promise<Record<string, unknown>> { const results = await database.getAll<Record<string, unknown>>(`SELECT * FROM ${table} WHERE id = ?`, [id]) - const result = results[0] ?? opData + const result = results[0] ?? (options?.allowPartial ? opData : undefined)
🤖 Fix all issues with AI agents
In @.claude/skills/code-simplifier/SKILL.md:
- Around line 59-61: The fenced code block containing "File:line - What changed
- Why it's simpler" lacks a language identifier; update the block delimiter from
``` to ```text (or another appropriate language) so markdown renderers apply
correct syntax highlighting and the document is compliant with fenced code block
rules — locate the fenced block in SKILL.md that wraps the line "File:line -
What changed - Why it's simpler" and add the language identifier immediately
after the opening backticks.
In @.vscode/tasks.json:
- Around line 4-15: The "ensure-postgres" task uses a POSIX-only wait loop and a
hard-coded container name; update the task (label "ensure-postgres") to use the
service-based command "docker compose exec -T postgres pg_isready -U gtdapp -d
gtdapp" instead of "docker exec gtdapp-postgres", replace the POSIX until/sleep
loop with an OS-aware wrapper (detect PowerShell/Windows vs POSIX shell) or use
a portable retry loop that calls the service-based pg_isready repeatedly, and
add a configurable timeout/maximum retries so the task fails instead of hanging
indefinitely.
In `@frontend/electron/auth.ts`:
- Around line 46-143: The startOAuthFlow flow must validate a nonce/state to
prevent local callback hijack: generate a random state (randomUUID from
node:crypto) in startOAuthFlow, include it as a query param in the authUrl and
persist it locally (e.g., in store or a closure), then when handling the
/callback in startOAuthFlow compare url.searchParams.get('state') to the saved
state before accepting the token (reject if mismatched). Also update the web
side (ElectronAuthPage.tsx) so when it redirects to the provided callback URL it
includes the same state param it received (pass through the state param from the
original auth URL into the redirect query) so the Electron side can validate it.
Ensure startOAuthFlow and ElectronAuthPage.tsx reference the same state name and
remove the stored state after successful validation.
- Around line 17-25: The code hardcodes an encryption key in the Store
initialization (Store<AuthStore> in auth.ts using the encryptionKey
'opengtd-electron-auth'), which exposes tokens at rest; replace this pattern by
removing the static encryptionKey and instead retrieving/storing secrets via an
OS-backed secret store: either use Electron's safeStorage or keytar to persist
the encryption key or, better, move sessionToken storage entirely into the OS
keychain and keep only a non-sensitive reference in the Store; update the
Store<AuthStore> initialization to omit encryptionKey and implement helper
functions (e.g., getSessionToken/setSessionToken) that read/write the token
through safeStorage/keytar and fall back to secure prompts if unavailable.
In `@frontend/package.json`:
- Around line 15-17: The build:win npm script currently hardcodes a WSL absolute
path in the electron-builder call ("build:win") which breaks cross-platform
builds; change the script to remove the
--config.directories.output=/mnt/c/temp/OpenGTD-release flag (or replace it with
a relative path like "dist" or "release"), and if you need a custom output
directory move that setting into the electron-builder config (e.g., package.json
"build" section or a separate electron-builder config file) using
"directories.output": "dist" so the build works on native Windows, WSL, and CI
without relying on /mnt/c paths or shell-specific parameter expansion.
In `@frontend/src/components/auth/ElectronAuthPage.tsx`:
- Around line 23-78: The callback URL validation in handleAuth() currently only
checks hostname and may allow non-HTTP(S) schemes to capture tokens; update the
validation of callbackUrl (the URL constructed in handleAuth) to ensure
url.protocol is "http:" or "https:" and reject other schemes, setting status
'error' and an appropriate error message. Also ensure you wait for the user data
to be fully loaded before redirecting: gate the redirect logic that reads
user?.id (after getToken() and before setting window.location.href) on the
useUser() load flag (e.g., user.isLoaded or a userIsLoaded variable) so you
don't send an empty user ID; if user data isn't loaded, set status to
'signing-in' or an error and return. Ensure these checks live inside the same
async handleAuth() flow that uses getToken({ template: 'electron' }) to avoid
racing.
In `@frontend/src/components/auth/ProtectedLayout.tsx`:
- Around line 54-70: Both async flows can leave loading flags set if the IPC
calls reject — update checkAuth and handleSignIn to wrap the await calls
(isElectronAuthenticated and startElectronOAuth) in try/catch/finally: in try
await the call, in catch set a user-facing error via setError(...) (for
checkAuth set an appropriate message or clear auth), and in finally ensure
setIsLoading(false) (for checkAuth) and setIsSigningIn(false) (for handleSignIn)
always run so the UI cannot remain stuck; preserve existing success-handling
logic inside the try block.
In `@frontend/src/features/actions/EditActionModal.tsx`:
- Around line 215-234: Recurrence DTO currently builds firstOccurrenceDate using
form.startDate?.toISOString().split('T')[0], which uses UTC and can shift the
local date; replace that expression with a local date-only formatter (using
form.startDate's getFullYear, getMonth+1, and getDate with zero-padding) or call
a shared helper (e.g., formatLocalDate) to produce "YYYY-MM-DD" from
form.startDate so recurrenceDto.firstOccurrenceDate reflects the user's local
calendar date.
In `@frontend/src/features/actions/useActions.ts`:
- Around line 344-350: The current race check queries ActionItems for status = 0
only, which misses instances in Waiting (status = 1); update the guard in the
transaction (where tx.getOptional is called for recurrencePatternId and id) to
treat any non-completed/non-cancelled item as existing—change the SQL predicate
to exclude completed/cancelled statuses (e.g., WHERE recurrencePatternId = ? AND
id != ? AND status NOT IN (COMPLETED_STATUS, CANCELLED_STATUS) or equivalent
using numeric values) so existingActive detects statuses 0 and 1 and prevents
spawning duplicates for the same recurrencePatternId.
In `@frontend/src/features/processing/ProcessPage.tsx`:
- Around line 467-505: The submit flow currently falls back to creating a
non‑recurring action when repeatEnabled is true but repeatPattern is falsy;
change the guard in the submit handler to prevent that by validating that
repeatPattern exists when repeatEnabled is true, and if missing reject the
submit (set form error / show UI error) instead of calling
createAction.mutateAsync; specifically update the block that checks
repeatEnabled and repeatPattern (and the subsequent use of
CreateRecurrencePattern and createRecurrence.mutateAsync) to throw or return
early with a user-facing error when repeatEnabled && !repeatPattern, leaving the
normal createAction.mutateAsync path only for when repeatEnabled is false.
In `@frontend/src/features/recurrence/RecurrenceEditor.tsx`:
- Around line 35-44: RecurrenceEditor currently only uses the incoming value
prop for initial useState calls (type, frequency, fixedInterval, daysOfWeek,
dayOfMonth, weekOfMonth, intervalValue, intervalUnit), so when the parent
changes value the internal form is stale; add a useEffect in RecurrenceEditor
that watches value (and/or value?.<fields>) and updates each of those state
setters (setType, setFrequency, setFixedInterval, setDaysOfWeek, setDayOfMonth,
setWeekOfMonth, setIntervalValue, setIntervalUnit) to mirror the new prop so the
editor stays in sync when the parent changes the edited item.
In `@frontend/src/features/recurrence/RecurrenceParser.ts`:
- Around line 264-358: The relative branch in formatRecurrence can output
"undefined ..." when pattern.intervalValue is missing; update formatRecurrence
to use a safe fallback (e.g. const value = pattern.intervalValue ?? 1) and use
that value for both the numeric part and plural calculation instead of directly
referencing pattern.intervalValue, so the returned string from formatRecurrence
(for RecurrenceType.Relative) always shows a valid number and correct
pluralization.
In `@frontend/src/features/recurrence/useRecurrence.ts`:
- Around line 128-181: The two separate inserts in useCreateRecurrencePattern
(mutate) must be executed inside a single atomic db.writeTransaction so
RecurrencePatterns and ActionItems are committed together; replace the two await
db.execute(...) calls with await db.writeTransaction(async (tx) => { await
tx.execute(...) for the RecurrencePatterns INSERT and await tx.execute(...) for
the ActionItems INSERT }) using the existing patternId and actionId variables,
and ensure any caught errors are rethrown after setIsPending is cleared so
callers receive the failure (i.e., keep the finally that calls
setIsPending(false) but do not swallow exceptions).
In `@frontend/src/lib/electron.ts`:
- Around line 53-95: The exported Electron helper functions assume
window.electronAPI.auth always exists; guard the auth surface and its methods
with optional chaining and safe defaults so calls won't throw when preload lacks
auth. Update startElectronOAuth, getElectronToken, getElectronUserId,
isElectronAuthenticated, and electronSignOut to call
window.electronAPI?.auth?.<method>() and return sensible fallbacks (undefined or
null for token/userId, false for isElectronAuthenticated, and a no-op/void for
electronSignOut) so older preloads/tests that omit auth fail safely.
In `@frontend/src/main.tsx`:
- Line 6: SignInPage and ElectronAuthPage call Clerk hooks (useSignIn, useAuth,
useUser) while being rendered in Electron without a ClerkProvider; add an
isElectron() guard in each component (SignInPage and ElectronAuthPage) to
short-circuit rendering and return a simple fallback UI when isElectron() is
true so the Clerk hooks never run, or alternatively add a router-level guard
that checks isElectron() and prevents those public auth routes from being
mounted in Electron; update the components/routes to use the isElectron() check
before any Clerk hook usage to avoid runtime errors.
In `@src/OpenGTD/Features/Inbox/InboxService.cs`:
- Around line 111-112: The code in InboxService currently preserves
client-provided IDs (Id = input.Id != Guid.Empty ? input.Id : Guid.NewGuid())
but doesn’t guard against conflicts; add a uniqueness check and a safe save
path: before calling SaveChangesAsync use the DbContext (e.g.,
_db.Inboxes.AnyAsync(i => i.Id == input.Id)) when input.Id != Guid.Empty and
return a controlled conflict/validation error if it exists, and also wrap
SaveChangesAsync in a try/catch for DbUpdateException to translate DB constraint
failures into the same conflict response; keep the Guid.Empty fallback behavior
intact.
In `@src/OpenGTD/Features/Recurrence/RecurrencePatternValidator.cs`:
- Around line 20-22: The RuleFor(x => x.FixedInterval).GreaterThan(0) validation
is applied unconditionally but should only run for fixed recurrences; update the
RecurrencePatternValidator to move that rule into the When(x => x.Type ==
RecurrenceType.Fixed, ...) block (and do the same for the duplicate rule around
lines 94-95) so FixedInterval is validated only when Type ==
RecurrenceType.Fixed, keeping other relative-type rules
(IntervalValue/IntervalUnit) unaffected.
🧹 Nitpick comments (14)
frontend/src/features/inbox/useInbox.ts (1)
109-116: Same null handling inconsistency; consider extracting shared mapping logic.This block has the same type casting issue as lines 49-56. Additionally, the mapping logic between
useInboxItems()anduseInboxItem()is duplicated.Extract a shared mapper function
function mapRowToInboxItem(row: InboxItemRow): InboxItem { return { id: row.id, title: row.title, description: row.description ?? undefined, type: (row.type ?? InboxItemType.Note) as InboxItem['type'], capturedAt: row.capturedAt ?? row.createdAt ?? new Date().toISOString(), sourcePath: row.sourcePath ?? undefined, attachmentCount: row.attachmentCount ?? 0, createdAt: row.createdAt ?? new Date().toISOString(), suggestedContextId: row.suggestedContextId ?? undefined, suggestedProjectId: row.suggestedProjectId ?? undefined, pendingProjectName: row.pendingProjectName ?? undefined, suggestedPriority: (row.suggestedPriority ?? undefined) as InboxItem['suggestedPriority'], suggestedTimeMinutes: row.suggestedTimeMinutes ?? undefined, suggestedDueDate: row.suggestedDueDate ?? undefined, suggestedDestination: (row.suggestedDestination ?? undefined) as InboxItem['suggestedDestination'], } }Then use it in both hooks:
// In useInboxItems: const items: InboxItem[] = data.map(mapRowToInboxItem) // In useInboxItem: const item: InboxItem | null = data[0] ? mapRowToInboxItem(data[0]) : nullfrontend/src/api/swagger.json (3)
1836-1854: Add tags + align PowerSync token operationId for codegen consistency.Nice to see an explicit
PowerSyncTokenResponseschema. To keep Swagger UI grouping and NSwag-generated method names consistent, consider tagging this endpoint and aligning theoperationIdwith the existing POST naming pattern.🧭 Suggested spec tweak
"/api/auth/powersync-token": { "post": { + "tags": [ + "Auth" + ], - "operationId": "GetPowerSyncToken", + "operationId": "PostApiAuthPowerSyncToken", "responses": {Also applies to: 3512-3518
1973-1978: MarkrecurrencePatternIdas readOnly if server-owned.The description implies this is system-assigned. Marking it as readOnly prevents accidental client writes in generated SDKs.
✅ Suggested spec tweak
"recurrencePatternId": { "type": "string", "description": "The recurrence pattern that created this task (if any).", "format": "guid", "nullable": true, + "readOnly": true }
2670-3293: Tighten recurrence DTO constraints + confirmformat: timesupport.RecurrencePattern defines min/max bounds (e.g., dayOfMonth 1–31, weekOfMonth 1–5, monthOfYear 1–12, intervalValue 1–365, maxOccurrences 1–1000, templateEstimatedMinutes 1–10080) and implies
fixedInterval >= 1, but the Create/Update/Sync DTOs don’t mirror these constraints or required fields (type/templateTitle/templatePriority). That can make generated clients accept invalid/incomplete payloads. Also,format: timeisn’t standard in OAS 3.0—please verify NSwag/client support or consider a pattern/string format.♻️ Suggested spec alignment (example)
"RecurrencePattern": { ... "fixedInterval": { "type": "integer", "description": "Interval between occurrences (e.g., every 2 weeks).\nDefault is 1. Only used when Type is Fixed.", "format": "int32", + "minimum": 1.0 }, ... }, "CreateRecurrencePatternDto": { "type": "object", "description": "DTO for creating or updating a recurrence pattern.", "additionalProperties": false, + "required": ["type", "templateTitle", "templatePriority"], "properties": { "fixedInterval": { "type": "integer", "format": "int32", + "minimum": 1.0 }, "dayOfMonth": { "type": "integer", "format": "int32", + "minimum": 1.0, + "maximum": 31.0, "nullable": true }Apply the same min/max constraints to
weekOfMonth,monthOfYear,intervalValue,maxOccurrences, andtemplateEstimatedMinutes, and mirror them inUpdateRecurrencePatternDtoandCreateRecurrencePatternFromSyncDto.frontend/src/styles/_pages.css (1)
69-92: Add a focus-visible style for section links (keyboard affordance).
Optional but helps keyboard users discover the link state in the new header UI.♿️ Suggested focus-visible styling
.theme-ink-refined .ir-section-link:hover { color: var(--ir-accent); } + +.theme-ink-refined .ir-section-link:focus-visible { + color: var(--ir-accent); + outline: 2px solid var(--ir-accent); + outline-offset: 2px; +}frontend/src/features/actions/actionUtils.ts (1)
25-35: Guard invalid deadlines to keep sorting deterministic.
If a malformed deadline sneaks in,Dateparsing yieldsNaN, which can make the comparator returnNaN. Consider treating invalid dates the same as missing dates.🛠️ Proposed hardening
export function sortByImportance<T extends { priority: number; deadline?: string | null }>(actions: T[]): T[] { return [...actions].sort((a, b) => { if (b.priority !== a.priority) return b.priority - a.priority - const aDeadline = a.deadline ? new Date(a.deadline).getTime() : Infinity - const bDeadline = b.deadline ? new Date(b.deadline).getTime() : Infinity + const aTime = a.deadline ? Date.parse(a.deadline) : NaN + const bTime = b.deadline ? Date.parse(b.deadline) : NaN + const aDeadline = Number.isFinite(aTime) ? aTime : Infinity + const bDeadline = Number.isFinite(bTime) ? bTime : Infinity return aDeadline - bDeadline }) }src/OpenGTD/Features/Recurrence/RecurrencePatternValidator.cs (1)
82-143: Consider extracting shared validation rules to reduce duplication.The
UpdateRecurrencePatternDtoValidatorduplicates all validation logic fromCreateRecurrencePatternDtoValidator. If both DTOs share a common base class or interface, you can extract the shared rules into a helper method or use FluentValidation'sInclude()method to compose validators.Example approach using a shared rule method
public class CreateRecurrencePatternDtoValidator : AbstractValidator<CreateRecurrencePatternDto> { public CreateRecurrencePatternDtoValidator() { ApplyCommonRules(this); } internal static void ApplyCommonRules<T>(AbstractValidator<T> validator) where T : IRecurrencePatternDto // Assuming a shared interface { validator.RuleFor(x => x.TemplateTitle) .NotEmpty().WithMessage("Title is required") .MaximumLength(500).WithMessage("Title cannot exceed 500 characters"); // ... other shared rules } } public class UpdateRecurrencePatternDtoValidator : AbstractValidator<UpdateRecurrencePatternDto> { public UpdateRecurrencePatternDtoValidator() { CreateRecurrencePatternDtoValidator.ApplyCommonRules(this); } }tests/OpenGTD.Tests/Helpers/TestDataBuilder.cs (1)
262-283: Align helper defaults with Fixed vs Relative patterns
CreateRecurrencePatternalways sets fixed-schedule fields even whentypeisRelative, and there’s no way to setIntervalValue/IntervalUnit. That can yield inconsistent test data for relative patterns. Consider conditional defaults and optional relative params.♻️ Suggested refactor
public static RecurrencePattern CreateRecurrencePattern( string templateTitle = "Test Recurring Task", RecurrenceType type = RecurrenceType.Fixed, - RecurrenceFrequency frequency = RecurrenceFrequency.Weekly, + RecurrenceFrequency? frequency = RecurrenceFrequency.Weekly, + int fixedInterval = 1, + DaysOfWeek? daysOfWeek = DaysOfWeek.Monday, + int? intervalValue = null, + IntervalUnit? intervalUnit = null, bool isActive = true) { - return new RecurrencePattern - { - TemplateTitle = templateTitle, - Type = type, - Frequency = frequency, - FixedInterval = 1, - DaysOfWeek = DaysOfWeek.Monday, - IsActive = isActive, - TemplatePriority = Priority.Normal, - CreatedAt = DateTime.UtcNow - }; + var pattern = new RecurrencePattern + { + TemplateTitle = templateTitle, + Type = type, + IsActive = isActive, + TemplatePriority = Priority.Normal, + CreatedAt = DateTime.UtcNow + }; + + if (type == RecurrenceType.Fixed) + { + pattern.Frequency = frequency; + pattern.FixedInterval = fixedInterval; + pattern.DaysOfWeek = frequency == RecurrenceFrequency.Weekly ? daysOfWeek : null; + } + else + { + pattern.IntervalValue = intervalValue ?? 1; + pattern.IntervalUnit = intervalUnit ?? IntervalUnit.Days; + } + + return pattern; }frontend/src/features/recurrence/RecurrenceField.tsx (1)
44-49: Clear the focus timeout to avoid stray callbacks.If enabled toggles quickly or the component unmounts, the pending timer can still fire. A small cleanup keeps this safe and more idiomatic.
♻️ Suggested tweak
- useEffect(() => { - if (enabled) { - setTimeout(() => inputRef.current?.focus(), 0) - } - }, [enabled]) + useEffect(() => { + if (!enabled) return + const timer = window.setTimeout(() => inputRef.current?.focus(), 0) + return () => window.clearTimeout(timer) + }, [enabled])src/OpenGTD/Features/Recurrence/RecurrenceEndpoints.cs (1)
16-18: Verify recurrence routes inherit authorization.If the parent group doesn’t already enforce auth, these endpoints may be exposed. Please confirm the group inherits authorization, or add it here.
🔐 Optional hardening (if not already enforced)
- var recurrence = group.MapGroup("/recurrence") - .WithTags("Recurrence"); + var recurrence = group.MapGroup("/recurrence") + .WithTags("Recurrence") + .RequireAuthorization();frontend/src/features/home/HomePage.tsx (1)
22-37: Avoid dual sources of truth for the edit modal.
showEditModalduplicateseditingActionId; deriving visibility from the id keeps state consistent and clears stale IDs on close.♻️ Suggested refactor
- const [showEditModal, setShowEditModal] = useState(false) const [editingActionId, setEditingActionId] = useState<string | null>(null) @@ - const handleEdit = (id: string) => { - setEditingActionId(id) - setShowEditModal(true) - } + const handleEdit = (id: string) => { + setEditingActionId(id) + } @@ - {showEditModal && ( + {editingActionId && ( <EditActionModal - show={showEditModal} - onClose={() => setShowEditModal(false)} + show + onClose={() => setEditingActionId(null)} actionId={editingActionId} initialAction={editingActionId ? nextActions.find((a) => a.id === editingActionId) : null} defaultIsNextAction={true} /> )}Also applies to: 124-131
frontend/src/types/schemas.ts (2)
197-199: Consider adding comments for all enum schemas.
RecurrenceTypeSchemahas a helpful comment. Adding similar comments forRecurrenceFrequencySchema(Daily=0, Weekly=1, Monthly=2, Yearly=3?) andIntervalUnitSchema(Days=0, Weeks=1, Months=2?) would improve maintainability.
254-254: UpdateRecurrencePatternSchema is inconsistent with other update schemas.Other update schemas like
UpdateActionItemSchemaandUpdateProjectSchemause.partial().extend({ id: z.string() }). This schema only uses.omit()without addingidor calling.partial(), meaning all fields fromCreateRecurrencePatternSchemaremain required.Consider aligning with the existing pattern:
-export const UpdateRecurrencePatternSchema = CreateRecurrencePatternSchema.omit({ firstOccurrenceDate: true }) +export const UpdateRecurrencePatternSchema = CreateRecurrencePatternSchema.omit({ firstOccurrenceDate: true }).partial().extend({ + id: z.string(), +})frontend/src/features/recurrence/useRecurrence.ts (1)
186-233: Update does not setmodifiedAttimestamp.The UPDATE statement doesn't include
modifiedAt = ?with the current timestamp. While the backend may set this on sync, the local SQLite database won't reflect when the pattern was last modified, which could affect local UI display or conflict resolution.Suggested fix
const mutate = async (id: string, dto: UpdateRecurrencePattern) => { setIsPending(true) try { + const now = new Date().toISOString() await db.execute( `UPDATE RecurrencePatterns SET type = ?, ... - templateIsNextAction = ? + templateIsNextAction = ?, + modifiedAt = ? WHERE id = ?`, [ dto.type, dto.frequency ?? null, ... - dto.templateIsNextAction ? 1 : 0, id + dto.templateIsNextAction ? 1 : 0, now, id ] )
- Add useMakeActionRecurring hook to create pattern and link existing action - Add useRemoveActionRecurrence hook to unlink action and deactivate pattern - EditActionModal now handles all 4 edit scenarios: - Update existing recurrence pattern - Remove recurrence from recurring action - Add recurrence to non-recurring action - No recurrence change - Fix ir-recurrence-trigger background to use var(--ir-surface) for consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security improvements: - Add OAuth state validation (CSRF protection) in electron auth flow - Replace hardcoded encryption key with OS keychain via safeStorage - Add protocol validation for callback URLs (http/https only) - Guard Clerk hooks with isElectron() checks to prevent runtime errors - Guard electron auth API methods with optional chaining Bug fixes: - Use local date formatter instead of UTC for recurrence dates - Fix race check to include Waiting status in duplicate prevention - Validate repeatPattern exists when repeatEnabled is true - Sync RecurrenceEditor state with prop changes via useEffect - Add safe fallback for intervalValue in formatRecurrence - Wrap recurrence pattern inserts in writeTransaction for atomicity - Add uniqueness check for client-provided inbox item IDs - Make FixedInterval validation conditional on recurrence type Config improvements: - Make postgres wait loop use service name and add 60s timeout - Make electron build output dir configurable via ELECTRON_OUTPUT_DIR - Add language identifier to SKILL.md fenced code block Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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 (2)
.vscode/tasks.json (1)
4-15: Good progress on timeout and service-based exec; Windows portability still missing.The changes address two of the three concerns from the previous review:
- ✓ Uses service name (
postgres) instead of hard-coded container name- ✓ Adds 60-second timeout to prevent infinite hangs
However, the command still uses POSIX-only constructs (
until,sleep,$((count+1))) that will fail on Windows (PowerShell/cmd). Consider adding awindowsoverride as previously suggested:🔧 Proposed fix to add Windows support
- "command": "docker compose up -d postgres && echo 'Waiting for PostgreSQL...' && timeout=60 && count=0 && until docker compose exec -T postgres pg_isready -U gtdapp -d gtdapp > /dev/null 2>&1; do sleep 1; count=$((count+1)); if [ $count -ge $timeout ]; then echo 'Timeout waiting for PostgreSQL'; exit 1; fi; done && echo 'PostgreSQL is ready'", + "command": "bash -c \"docker compose up -d postgres && echo 'Waiting for PostgreSQL...' && timeout=60 && count=0 && until docker compose exec -T postgres pg_isready -U gtdapp -d gtdapp > /dev/null 2>&1; do sleep 1; count=$((count+1)); if [ $count -ge $timeout ]; then echo 'Timeout waiting for PostgreSQL'; exit 1; fi; done && echo 'PostgreSQL is ready'\"", + "windows": { + "command": "powershell -NoProfile -Command \"docker compose up -d postgres; Write-Host 'Waiting for PostgreSQL...'; $count=0; while ($count -lt 60 -and -not (docker compose exec -T postgres pg_isready -U gtdapp -d gtdapp 2>$null)) { Start-Sleep -Seconds 1; $count++ }; if ($count -ge 60) { Write-Host 'Timeout waiting for PostgreSQL'; exit 1 }; Write-Host 'PostgreSQL is ready'\"" + },frontend/electron/auth.ts (1)
116-130: Escape the error message to prevent potential XSS.The
errorparameter from the URL is interpolated directly into HTML on line 122. While this is a localhost callback, a malicious actor who can trigger a redirect to this callback could inject script content.🔒 Suggested fix
if (error) { res.writeHead(200, { 'Content-Type': 'text/html' }) + const safeError = error.replace(/[&<>"']/g, (c) => ({ + '&': '&', + '<': '<', + '>': '>', + '"': '"', + "'": ''' + }[c] || c)) res.end(` <html> <body style="font-family: system-ui; padding: 40px; text-align: center;"> <h1>Authentication Failed</h1> - <p>${error}</p> + <p>${safeError}</p> <p>You can close this window.</p> </body> </html> `)
🤖 Fix all issues with AI agents
In `@frontend/src/components/auth/ElectronAuthPage.tsx`:
- Around line 19-38: The early return for isElectron() is placed before React
hooks are called in ElectronAuthPage, violating the Rules of Hooks; fix by
ensuring hooks run unconditionally — either move all hook calls (useAuth,
useUser, useSearchParams, useState) to the top of ElectronAuthPage before the
isElectron() guard and then conditionally render the error UI, or split into two
components: keep the isElectron() check in ElectronAuthPage and render a new
ElectronAuthPageImpl that contains the hooks and the existing implementation.
In `@frontend/src/components/auth/SignInPage.tsx`:
- Around line 24-40: The conditional early return in SignInPage causes
useAuth/useSignIn/useSignUp hooks to be called conditionally; move the hook
calls (useAuth, useSignIn, useSignUp) to the top of the SignInPage function
before the isElectron() guard so hooks are always invoked in the same order,
then keep the Electron fallback return after those hooks and render the normal
sign-in UI when not Electron.
In `@frontend/src/features/actions/EditActionModal.tsx`:
- Around line 287-290: In EditActionModal.tsx, verify whether the API expects
date-only strings or full ISO timestamps for non-recurring actions; if
date-only, replace the toISOString() calls for startDate, dueDate, and deadline
in the non-recurring payload with the existing formatLocalDate(form.startDate),
formatLocalDate(form.dueDate), and formatLocalDate(form.deadline); if the
backend requires timestamps, leave them as toISOString() but add a comment
clarifying the choice. Ensure you update the payload construction inside the
submit/serialize path in the EditActionModal component where
startDate/dueDate/deadline are set.
In `@frontend/src/features/recurrence/RecurrenceEditor.tsx`:
- Around line 45-55: The effect syncing prop "value" to local state (useEffect)
only updates when value fields are defined, which leaves stale state when
switching types; modify the effect in RecurrenceEditor to explicitly reset
fields to their default/empty values when the incoming value's properties are
undefined (e.g., call setFrequency(undefined or ''),
setFixedInterval(undefined), setDaysOfWeek([]), setDayOfMonth(undefined),
setWeekOfMonth(undefined), setIntervalValue(undefined),
setIntervalUnit(undefined), and setType(value?.type ?? defaultType)) so every
relevant setter (setType, setFrequency, setFixedInterval, setDaysOfWeek,
setDayOfMonth, setWeekOfMonth, setIntervalValue, setIntervalUnit) is invoked
with either the incoming value or a cleared default to avoid retaining stale
values.
In `@frontend/src/features/recurrence/useRecurrence.ts`:
- Around line 333-357: The two DB updates in useRemoveActionRecurrence (mutate)
must be executed atomically: open a transaction on the usePowerSync DB, perform
both db.execute calls (the UPDATE ActionItems ... and UPDATE RecurrencePatterns
...) inside that transaction, then COMMIT; on any error ROLLBACK and rethrow so
state stays consistent; keep setIsPending(false) in the finally block and ensure
you use the DB's transaction API (or explicit BEGIN/COMMIT/ROLLBACK) rather than
separate executes so both updates succeed or neither do.
- Around line 362-382: The delete logic in useDeleteRecurrencePattern is not
atomic—clear FK references with db.execute and the DELETE must be wrapped in a
transaction so both succeed or both roll back; update mutate (in
useDeleteRecurrencePattern) to begin a transaction, run the UPDATE and DELETE
inside it (using the existing db transaction API or explicit
BEGIN/COMMIT/ROLLBACK via db.execute), commit on success, rollback and rethrow
on error, and ensure setIsPending(false) still runs in finally.
- Around line 285-328: The two DB operations in useMakeActionRecurring (the
INSERT into RecurrencePatterns and the UPDATE of ActionItems using patternId)
must be executed inside a single transaction so they either both succeed or both
roll back; modify the mutate function to begin a transaction before the first
db.execute, perform the INSERT and UPDATE, then COMMIT on success and ROLLBACK
on any error (catch, rollback, rethrow) so you don't leave an orphan pattern;
use the same db API (e.g., db.execute('BEGIN'), 'COMMIT', 'ROLLBACK' or the
db.transaction helper if available) and keep the existing finally block to
setIsPending(false).
🧹 Nitpick comments (6)
src/OpenGTD/Features/Recurrence/RecurrencePatternValidator.cs (1)
81-141: Consider extracting shared validation rules to reduce duplication.Both
CreateRecurrencePatternDtoValidatorandUpdateRecurrencePatternDtoValidatorhave identical rule sets. FluentValidation'sInclude()method can share rules between validators.♻️ Suggested refactor using Include()
// Create a shared rules class public class RecurrencePatternRules : AbstractValidator<IRecurrencePatternDto> { public RecurrencePatternRules() { RuleFor(x => x.TemplateTitle) .NotEmpty().WithMessage("Title is required") .MaximumLength(500).WithMessage("Title cannot exceed 500 characters"); // ... all shared rules } } // Then in each validator: public class CreateRecurrencePatternDtoValidator : AbstractValidator<CreateRecurrencePatternDto> { public CreateRecurrencePatternDtoValidator() { Include(new RecurrencePatternRules()); // Add any Create-specific rules here } }Alternatively, define a common interface
IRecurrencePatternDtothat both DTOs implement.tests/OpenGTD.Tests/Features/Recurrence/RecurrenceServiceTests.cs (1)
194-218: Consider adding a verification thatUpdateFromSyncAsynclogs the creation.The
UpdateFromSyncAsync_CreatesWhenMissingtest verifies the entity is created, but per the service docstring, it also logs when creating from a missing pattern. Consider verifying the logger was called to ensure the fallback path is fully covered.💡 Optional: Verify logging behavior
// After asserting the entity was created, verify the log call: _loggerMock.Verify( x => x.Log( LogLevel.Information, It.IsAny<EventId>(), It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("not found") || v.ToString()!.Contains("Creating")), null, It.IsAny<Func<It.IsAnyType, Exception?, string>>()), Times.Once);frontend/src/features/recurrence/RecurrenceParser.ts (1)
321-330:getDayNameonly returns the first matching day for multi-day patterns.When
daysOfWeekhas multiple days set (e.g., Monday | Wednesday),getDayNamereturns only the first match due to the early-return pattern. This may produce incomplete displays like "every Monday" when the user selected multiple days.Consider handling multi-day patterns for more accurate display:
💡 Multi-day formatting suggestion
function getDayName(day: DaysOfWeek): string { const days: string[] = [] if (day & DaysOfWeek.Sunday) days.push('Sunday') if (day & DaysOfWeek.Monday) days.push('Monday') if (day & DaysOfWeek.Tuesday) days.push('Tuesday') if (day & DaysOfWeek.Wednesday) days.push('Wednesday') if (day & DaysOfWeek.Thursday) days.push('Thursday') if (day & DaysOfWeek.Friday) days.push('Friday') if (day & DaysOfWeek.Saturday) days.push('Saturday') return days.join(', ') || '' }frontend/src/features/recurrence/RecurrenceEditor.tsx (1)
74-76: Effect emits changes on every dependency update, which may cause unnecessary callbacks.The
useEffectat lines 74-76 callsemitChange()whenever any form state changes. This is correct for notifying the parent, but if the parent'sonChangeupdatesvalue, and the sync effect (lines 45-55) then updates local state, this could cause multiple emissions per user interaction.This is likely acceptable given the conditional sync logic, but consider debouncing or comparing with previous emitted value if performance issues arise.
frontend/src/features/actions/useActions.ts (1)
388-403: Hardcoded priority default (2) should use the enum constant.Line 393 uses
pattern.templatePriority ?? 2as a fallback. While2likely corresponds toPriority.Normal, using the enum constant would be more maintainable and self-documenting.💡 Use enum constant
- pattern.templatePriority ?? 2, + pattern.templatePriority ?? Priority.Normal,Note: This requires importing
Priorityif not already available in scope.frontend/src/features/processing/ProcessPage.tsx (1)
475-521: Inconsistent date format between recurrence and regular action paths.Recurrence uses local
YYYY-MM-DDformat viaformatLocalDate(line 502), while regular actions usetoISOString()which outputs UTC (lines 516-518). This inconsistency could cause off-by-one-day errors depending on the user's timezone and how the backend interprets these dates.Consider unifying the date formatting approach. If the backend expects local dates for scheduling purposes, the regular action path should also use
formatLocalDate.♻️ Suggested unification
} else { // Create a regular (non-recurring) action const dto: CreateActionItem = { title: actionForm.title, description: actionForm.description || undefined, contextId: actionForm.contextId, projectId: actionForm.projectId, priority: actionForm.priority, energyLevel: actionForm.energyLevel, estimatedMinutes: actionForm.estimatedMinutes, isNextAction: actionForm.isNextAction, - startDate: actionForm.startDate?.toISOString(), - dueDate: actionForm.dueDate?.toISOString(), - deadline: actionForm.deadline?.toISOString(), + startDate: formatLocalDate(actionForm.startDate), + dueDate: formatLocalDate(actionForm.dueDate), + deadline: formatLocalDate(actionForm.deadline), } await createAction.mutateAsync(dto)Please verify whether the backend expects ISO timestamps or local date strings for
CreateActionItemdate fields.
- ElectronAuthPage: Move hooks before isElectron() check to comply with Rules of Hooks - SignInPage: Move hooks before isElectron() check to comply with Rules of Hooks - RecurrenceEditor: Reset all fields to defaults in useEffect to avoid stale state - useRecurrence: Wrap useMakeActionRecurring, useRemoveActionRecurrence, and useDeleteRecurrencePattern in writeTransaction for atomicity - EditActionModal: Use formatLocalDate consistently for all date fields to avoid UTC timezone shifts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix getDayName to handle multi-day patterns (e.g., "Monday, Wednesday") - Replace magic number 2 with Priority.Normal enum constant - Unify date formatting to use local dates, preventing timezone shifts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/OpenGTD/Features/Inbox/InboxService.cs`:
- Around line 165-171: The InboxItem construction uses
parseResult.Title/RawTitle which is only defined when clientParsed is false;
update the initializer to use finalTitle for Title and compute RawTitle by
comparing input.Title to finalTitle (store input.Title only if different) so the
code compiles for both branches—modify the InboxItem initializer in the method
that defines parseResult, clientParsed and finalTitle to set Title = finalTitle
and RawTitle = input.Title != finalTitle ? input.Title : null while keeping Id,
Description, etc. unchanged.
♻️ Duplicate comments (1)
src/OpenGTD/Features/Inbox/InboxService.cs (1)
153-188: Handle TOCTOU on duplicate IDs during save.The pre-check reduces collisions, but a concurrent insert can still trigger a
DbUpdateExceptiononSaveChangesAsync. Consider catching it and translating to the same conflict response to avoid leaking unhandled DB errors.
parseResult is only defined in the server-parsing branch, but the InboxItem constructor was referencing it unconditionally. Use finalTitle which is set in both client-parsed and server-parsed branches. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/auth/ElectronAuthPage.tsx`:
- Around line 29-40: The useEffect hooks in ElectronAuthPage are being invoked
conditionally due to the early return guard (isElectron()), violating Rules of
Hooks; fix this by splitting the component into a simple wrapper
(ElectronAuthPage) that checks isElectron() and returns the error UI early, and
a separate implementation component (e.g., ElectronAuthPageImpl) that contains
all hooks (useEffect, useAuth, useUser, useSearchParams, useState) and the rest
of the JSX; have the wrapper render <ElectronAuthPageImpl /> when not in
Electron so hooks are always called unconditionally inside the implementation
component.
- Around line 90-94: In ElectronAuthPage's effect that checks auth state, don't
setStatus('signing-in') when a token exists but user?.id is not yet loaded
because that triggers an unwanted redirect; instead, when token is present and
user?.id is falsy simply return (or set a neutral status like 'loading-user')
and wait for user to populate so the effect re-runs and handles the redirect
only once user data is actually available; update the logic around the setStatus
call that currently runs when !user?.id to only run when there is no token (or
explicitly signing out).
🧹 Nitpick comments (2)
frontend/src/features/recurrence/RecurrenceParser.ts (1)
168-193: Consider adding support for "every N years" pattern.The interval pattern handles "every N days/weeks/months" but not "every N years". While "yearly" is supported, users might expect "every 2 years" to work.
♻️ Optional enhancement
- const intervalMatch = text.match(/^every\s+(\d+)\s+(days?|weeks?|months?)$/i) + const intervalMatch = text.match(/^every\s+(\d+)\s+(days?|weeks?|months?|years?)$/i) if (intervalMatch) { const interval = parseInt(intervalMatch[1], 10) const unit = intervalMatch[2].toLowerCase() if (unit.startsWith('day')) { return { type: RecurrenceType.Fixed, frequency: RecurrenceFrequency.Daily, fixedInterval: interval, } } else if (unit.startsWith('week')) { return { type: RecurrenceType.Fixed, frequency: RecurrenceFrequency.Weekly, fixedInterval: interval, } } else if (unit.startsWith('month')) { return { type: RecurrenceType.Fixed, frequency: RecurrenceFrequency.Monthly, fixedInterval: interval, } + } else if (unit.startsWith('year')) { + return { + type: RecurrenceType.Fixed, + frequency: RecurrenceFrequency.Yearly, + fixedInterval: interval, + } } }frontend/src/features/recurrence/useRecurrence.ts (1)
186-236: Consider updatingmodifiedAttimestamp on pattern updates.The UPDATE statement doesn't set
modifiedAt, which may affect sync behavior or audit trails. Other update operations in the codebase (e.g., inuseCompleteAction) setmodifiedAt = ?with the current timestamp.♻️ Suggested fix
const mutate = async (id: string, dto: UpdateRecurrencePattern) => { setIsPending(true) try { + const now = new Date().toISOString() await db.execute( `UPDATE RecurrencePatterns SET type = ?, frequency = ?, fixedInterval = ?, daysOfWeek = ?, dayOfMonth = ?, weekOfMonth = ?, monthOfYear = ?, intervalValue = ?, intervalUnit = ?, startTime = ?, endDate = ?, maxOccurrences = ?, templateTitle = ?, templateDescription = ?, templateContextId = ?, templateProjectId = ?, templatePriority = ?, templateEnergyLevel = ?, templateEstimatedMinutes = ?, - templateIsNextAction = ? + templateIsNextAction = ?, + modifiedAt = ? WHERE id = ?`, [ dto.type, dto.frequency ?? null, dto.fixedInterval ?? 1, dto.daysOfWeek ?? null, dto.dayOfMonth ?? null, dto.weekOfMonth ?? null, dto.monthOfYear ?? null, dto.intervalValue ?? null, dto.intervalUnit ?? null, dto.startTime ?? null, dto.endDate ?? null, dto.maxOccurrences ?? null, dto.templateTitle, dto.templateDescription ?? null, dto.templateContextId ?? null, dto.templateProjectId ?? null, dto.templatePriority ?? 2, dto.templateEnergyLevel ?? null, dto.templateEstimatedMinutes ?? null, - dto.templateIsNextAction ? 1 : 0, id + dto.templateIsNextAction ? 1 : 0, now, id ] )
Comment on lines
+29
to
40
| // This page should only run in the browser (opened by Electron), not in Electron itself | ||
| if (isElectron()) { | ||
| return ( | ||
| <div style={{ padding: '40px', textAlign: 'center', fontFamily: 'system-ui' }}> | ||
| <h1>Configuration Error</h1> | ||
| <p>This page should open in your browser, not the app.</p> | ||
| <p>Please close this and try again.</p> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Hooks still called conditionally after early return, violating Rules of Hooks.
The early return at lines 30-38 causes the useEffect hooks at lines 40 and 116 to be called conditionally. While the previous review addressed moving useAuth, useUser, useSearchParams, and useState before the guard, the useEffect hooks remain after it. Biome correctly flags this violation.
The cleanest fix is to split into wrapper and implementation components:
Suggested fix: Split into wrapper and implementation components
+function ElectronAuthPageImpl() {
+ const { isSignedIn, isLoaded, getToken } = useAuth()
+ const { user } = useUser()
+ const [searchParams] = useSearchParams()
+ const [status, setStatus] = useState<'loading' | 'signing-in' | 'getting-token' | 'redirecting' | 'error'>('loading')
+ const [error, setError] = useState<string | null>(null)
+
+ const callbackUrl = searchParams.get('callback')
+ const oauthState = searchParams.get('state')
+
+ useEffect(() => {
+ // ... existing auth logic
+ }, [isLoaded, isSignedIn, callbackUrl, getToken, user?.id, oauthState])
+
+ useEffect(() => {
+ // ... existing sign-in redirect logic
+ }, [status, callbackUrl, oauthState])
+
+ // ... existing render logic
+}
+
export function ElectronAuthPage() {
- // Hooks must be called unconditionally (Rules of Hooks)
- const { isSignedIn, isLoaded, getToken } = useAuth()
- const { user } = useUser()
- const [searchParams] = useSearchParams()
- const [status, setStatus] = useState<'loading' | 'signing-in' | 'getting-token' | 'redirecting' | 'error'>('loading')
- const [error, setError] = useState<string | null>(null)
-
- const callbackUrl = searchParams.get('callback')
- const oauthState = searchParams.get('state')
-
- // This page should only run in the browser (opened by Electron), not in Electron itself
if (isElectron()) {
return (
<div style={{ padding: '40px', textAlign: 'center', fontFamily: 'system-ui' }}>
<h1>Configuration Error</h1>
<p>This page should open in your browser, not the app.</p>
<p>Please close this and try again.</p>
</div>
)
}
-
- useEffect(() => {
- // ...
+ return <ElectronAuthPageImpl />
+}Also applies to: 116-116
🧰 Tools
🪛 Biome (2.1.2)
[error] 40-40: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In `@frontend/src/components/auth/ElectronAuthPage.tsx` around lines 29 - 40, The
useEffect hooks in ElectronAuthPage are being invoked conditionally due to the
early return guard (isElectron()), violating Rules of Hooks; fix this by
splitting the component into a simple wrapper (ElectronAuthPage) that checks
isElectron() and returns the error UI early, and a separate implementation
component (e.g., ElectronAuthPageImpl) that contains all hooks (useEffect,
useAuth, useUser, useSearchParams, useState) and the rest of the JSX; have the
wrapper render <ElectronAuthPageImpl /> when not in Electron so hooks are always
called unconditionally inside the implementation component.
Comment on lines
+90
to
+94
| // Wait for user data to be loaded before redirecting | ||
| if (!user?.id) { | ||
| setStatus('signing-in') | ||
| return | ||
| } |
There was a problem hiding this comment.
Setting status to 'signing-in' when user data isn't loaded may cause unnecessary redirect.
When the token is successfully obtained but user?.id is not yet available, setting status to 'signing-in' triggers a redirect to the sign-in page—even though the user is already signed in. Since user?.id is in the effect dependencies, the effect will re-run when user data loads, but by then the redirect may have already started.
Consider waiting for user data instead:
Suggested fix
// Wait for user data to be loaded before redirecting
if (!user?.id) {
- setStatus('signing-in')
+ // User data still loading, effect will re-run when user?.id changes
return
}🤖 Prompt for AI Agents
In `@frontend/src/components/auth/ElectronAuthPage.tsx` around lines 90 - 94, In
ElectronAuthPage's effect that checks auth state, don't setStatus('signing-in')
when a token exists but user?.id is not yet loaded because that triggers an
unwanted redirect; instead, when token is present and user?.id is falsy simply
return (or set a neutral status like 'loading-user') and wait for user to
populate so the effect re-runs and handles the redirect only once user data is
actually available; update the logic around the setStatus call that currently
runs when !user?.id to only run when there is no token (or explicitly signing
out).
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.