Desktop App Code Reviews #32
Conversation
📝 WalkthroughWalkthroughStandardizes wikilink handling, renames many preference fields to camelCase, splits the monolithic vault service into modular vault packages (keys/queries/mutations/utils/init), adds task-order hook factories and tests, extracts deep-link and calendar DnD logic into hooks, adds tag-input validation callback, and centralizes Rust status serialization. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* Turn future tasks into GH Issues * Update task doc * Vault Switching WIP * Sort vault switching * Complete vault switching work
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tdn-desktop/src/lib/wikilink.ts`:
- Around line 46-49: isWikilink currently returns true for an empty wikilink
"[[]]" whereas extractWikilinkTitle returns null, causing downstream functions
like ensureWikilink and stripWikilink to receive inconsistent inputs; update
isWikilink to only return true when a valid title exists by either calling
extractWikilinkTitle(value) and checking for non-null, or by using the same
validation regex/pattern used by extractWikilinkTitle (e.g., require at least
one non-bracket character between [[ and ]]) so both functions agree on what
constitutes a valid wikilink.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
tdn-desktop/docs/tasks-done/task-2026-01-12-1-markdown-editor-improvements.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-done/task-2026-01-16-9b-rust-eview.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-done/task-2026-01-16-9c-react-review.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9c-react-review.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-clean-code-review.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-findings-react-components.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-findings-react-core.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-findings-rust.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-findings-synthesis.mdis excluded by!**/*.mdtdn-desktop/docs/tasks-todo/task-9d-review-implementation-plan.mdis excluded by!**/*.md
📒 Files selected for processing (12)
tdn-desktop/src/components/kanban/KanbanColumn.tsxtdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/components/views/WeekView.tsxtdn-desktop/src/hooks/use-command-context.tstdn-desktop/src/lib/commands/task-commands.tstdn-desktop/src/lib/wikilink.test.tstdn-desktop/src/lib/wikilink.tstdn-desktop/src/services/preferences.tstdn-desktop/src/store/view-mode-store.ts
💤 Files with no reviewable changes (1)
- tdn-desktop/src/components/kanban/KanbanColumn.tsx
✅ Files skipped from review due to trivial changes (1)
- tdn-desktop/src/hooks/use-command-context.ts
🧰 Additional context used
📓 Path-based instructions (6)
tdn-desktop/**/*.{tsx,ts}
📄 CodeRabbit inference engine (tdn-desktop/AGENTS.md)
tdn-desktop/**/*.{tsx,ts}: Use selector syntax in Zustand hooks to access state (e.g.,useUIStore(state => state.leftSidebarVisible)), never destructure from Zustand hooks
Use getState() in callbacks to access Zustand state instead of destructuring
Use type-safe Tauri commands from@/lib/tauri-bindings(tauri-specta) with Result handling, never use string-based invoke()
Files:
tdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/services/preferences.tstdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/lib/wikilink.tstdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/store/view-mode-store.tstdn-desktop/src/lib/wikilink.test.tstdn-desktop/src/lib/commands/task-commands.tstdn-desktop/src/components/views/WeekView.tsx
tdn-desktop/**/*.tsx
📄 CodeRabbit inference engine (tdn-desktop/AGENTS.md)
tdn-desktop/**/*.tsx: Use useTranslation hook from react-i18next in React components for internationalization
Place all user-facing strings in /locales/*.json translation files
Use React Compiler for automatic memoization handling (no manual useMemo/useCallback needed)
Files:
tdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/components/views/WeekView.tsx
tdn-desktop/**/*.{tsx,css,scss}
📄 CodeRabbit inference engine (tdn-desktop/AGENTS.md)
Use CSS logical properties (e.g., text-start instead of text-left) for RTL support
Files:
tdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/components/views/WeekView.tsx
tdn-desktop/**/*.{rs,tsx,ts}
📄 CodeRabbit inference engine (tdn-desktop/AGENTS.md)
Emit events from Rust to React using app.emit("event-name", data) and listen with listen("event-name", handler) in React
Files:
tdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/services/preferences.tstdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/lib/wikilink.tstdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/store/view-mode-store.tstdn-desktop/src/lib/wikilink.test.tstdn-desktop/src/lib/commands/task-commands.tstdn-desktop/src/components/views/WeekView.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Where appropriate, task/project/area file handling must conform to tdn-specs/S2-interface-design.md and tdn-specs/S3-data-read-write.md specifications
Files:
tdn-desktop/src/components/preferences/panes/VaultPane.tsxtdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/services/preferences.tstdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/lib/wikilink.tstdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/store/view-mode-store.tstdn-desktop/src/lib/wikilink.test.tstdn-desktop/src/lib/commands/task-commands.tstdn-desktop/src/components/views/WeekView.tsx
tdn-desktop/**/*.ts
📄 CodeRabbit inference engine (tdn-desktop/AGENTS.md)
Use i18n.t.bind(i18n) for non-React contexts or call i18n.t() directly for occasional use
Files:
tdn-desktop/src/services/preferences.tstdn-desktop/src/lib/wikilink.tstdn-desktop/src/store/view-mode-store.tstdn-desktop/src/lib/wikilink.test.tstdn-desktop/src/lib/commands/task-commands.ts
🧠 Learnings (6)
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/src/**/*.{ts,tsx} : Use path aliases in TypeScript imports: `@/*` maps to `src/*` (e.g., `import { foo } from '@/commands/list'`) and `bindings` maps to `bindings/` (e.g., `import { parseTask } from 'bindings`)
Applied to files:
tdn-desktop/src/components/tasks/TaskDetailPanel.tsxtdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/lib/wikilink.tstdn-desktop/src/components/preferences/panes/QuickEntryPane.tsxtdn-desktop/src/lib/commands/task-commands.tstdn-desktop/src/components/views/WeekView.tsx
📚 Learning: 2026-01-16T00:27:24.860Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T00:27:24.860Z
Learning: Applies to **/*.{ts,tsx,js} : Where appropriate, task/project/area file handling must conform to tdn-specs/S2-interface-design.md and tdn-specs/S3-data-read-write.md specifications
Applied to files:
tdn-desktop/src/components/views/ProjectView.tsx
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: Applies to tdn-desktop/**/*.{tsx,ts} : Use type-safe Tauri commands from `@/lib/tauri-bindings` (tauri-specta) with Result handling, never use string-based invoke()
Applied to files:
tdn-desktop/src/components/views/ProjectView.tsxtdn-desktop/src/lib/commands/task-commands.ts
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: Applies to tdn-desktop/**/*.{tsx,ts} : Use selector syntax in Zustand hooks to access state (e.g., `useUIStore(state => state.leftSidebarVisible)`), never destructure from Zustand hooks
Applied to files:
tdn-desktop/src/store/view-mode-store.ts
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: State management follows the onion pattern: useState (component) → Zustand (global UI) → TanStack Query (persistent data)
Applied to files:
tdn-desktop/src/store/view-mode-store.ts
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: Applies to tdn-desktop/**/*.{tsx,ts} : Use getState() in callbacks to access Zustand state instead of destructuring
Applied to files:
tdn-desktop/src/store/view-mode-store.ts
🧬 Code graph analysis (7)
tdn-desktop/src/components/preferences/panes/VaultPane.tsx (1)
tdn-desktop/src/services/preferences.ts (1)
preferencesQueryKeys(7-12)
tdn-desktop/src/components/tasks/TaskDetailPanel.tsx (1)
tdn-desktop/src/lib/wikilink.ts (1)
stripWikilink(72-74)
tdn-desktop/src/components/views/ProjectView.tsx (1)
tdn-desktop/src/lib/wikilink.ts (1)
stripWikilink(72-74)
tdn-desktop/src/components/preferences/panes/QuickEntryPane.tsx (1)
tdn-desktop/src/services/preferences.ts (1)
preferencesQueryKeys(7-12)
tdn-desktop/src/lib/wikilink.test.ts (1)
tdn-desktop/src/lib/wikilink.ts (4)
extractWikilinkTitle(25-41)isWikilink(46-49)ensureWikilink(55-58)stripWikilink(72-74)
tdn-desktop/src/lib/commands/task-commands.ts (1)
tdn-desktop/src/lib/wikilink.ts (1)
stripWikilink(72-74)
tdn-desktop/src/components/views/WeekView.tsx (1)
tdn-desktop/src/lib/wikilink.ts (1)
stripWikilink(72-74)
🔇 Additional comments (21)
tdn-desktop/src/store/view-mode-store.ts (1)
2-2: Persisted view-mode store wiring looks good.
Please sanity-check thatview-mode-storageis the intended key and that persistence/hydration behaves as expected in the Tauri webview environment.Also applies to: 48-61
tdn-desktop/src/services/preferences.ts (1)
7-11: Centralized preference query keys are a nice cleanup.
Please confirm the new keys align with your react-query invalidation/usage patterns across the app to avoid collisions.tdn-desktop/src/components/preferences/panes/VaultPane.tsx (1)
13-17: Nice consistency improvement using the shared dev-mode key.
Please double-check any cache invalidations or prefetches use the same key factory.Also applies to: 32-34
tdn-desktop/src/components/preferences/panes/QuickEntryPane.tsx (1)
10-14: Good move to the shared default-shortcut key.
Please ensure any related cache updates/invalidation use the same key factory.Also applies to: 26-28
tdn-desktop/src/lib/wikilink.ts (3)
13-41: Clear, readable parsing logic.
The alias/heading handling is straightforward and matches the documented examples.
51-58:ensureWikilinkbehavior looks consistent.
Nice and minimal helper.
60-74:stripWikilinkis a good convenience wrapper.
The fallback to trimmed input is sensible for non-wikilink values.tdn-desktop/src/lib/commands/task-commands.ts (2)
20-20: LGTM!Good refactor to use the centralized
stripWikilinkutility from@/lib/wikilinkinstead of a local helper, promoting code reuse across the codebase.
207-208: LGTM!The migration to
stripWikilinkcorrectly extracts the title/ID from wikilink format (e.g.,[[Project Name]]→Project Name) before passing tocreateTask. This aligns with the shared utility pattern adopted across other components.tdn-desktop/src/lib/wikilink.test.ts (5)
1-7: LGTM!Clean imports and good organization of the test file, importing all four utility functions being tested.
9-68: Thorough test coverage forextractWikilinkTitle.The tests comprehensively cover basic wikilinks, aliases, headings, combinations, non-wikilinks, invalid formats, and whitespace handling. Edge cases like heading-only (
[[#Heading]]) and alias-only ([[|Alias]]) are appropriately tested to returnnull.
70-88: LGTM!Good coverage for
isWikilinkincluding valid formats, non-wikilinks, malformed brackets, and whitespace handling.
90-105: LGTM!Tests for
ensureWikilinkproperly verify wrapping plain text, preserving existing wikilinks, and trimming whitespace.
107-129: LGTM!Tests for
stripWikilinkcorrectly verify extraction, fallback to original string for non-wikilinks, whitespace trimming, and preservation of invalid wikilinks like[[]]and[[#Heading]].tdn-desktop/src/components/tasks/TaskDetailPanel.tsx (2)
6-6: LGTM!Correct import of the shared
stripWikilinkutility using the proper path alias.
99-109: LGTM!Good refactor of
findProjectByWikilinkandfindAreaByWikilinkto use the centralizedstripWikilinkutility. The lookup logic correctly searches by both title and id, handling cases where the wikilink content could be either.tdn-desktop/src/components/views/ProjectView.tsx (2)
9-9: LGTM!Correct import of the shared
stripWikilinkutility.
102-109: LGTM!Good refactor of
getAreaNameto use the centralizedstripWikilinkutility. The dependency array correctly includes onlyareassincestripWikilinkis a stable module-level import.tdn-desktop/src/components/views/WeekView.tsx (3)
5-5: LGTM!Correct import of the shared
stripWikilinkutility.
102-119: LGTM!Good refactor of
getProjectNameandgetAreaNameto usestripWikilink. Dependency arrays correctly updated to remove the previously-included localextractTitlesincestripWikilinkis a stable module-level import.
290-311: LGTM!Navigation handlers
handleKanbanProjectClickandhandleKanbanAreaClickcorrectly migrated to usestripWikilink. Dependency arrays properly updated to[projects, navigate]and[areas, navigate]respectively.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tdn-desktop/src-tauri/src/types.rs (1)
28-52: Add backward-compatible field aliases for legacy snake_case preferences.The
rename_all = "camelCase"attribute will only deserialize camelCase keys from JSON. If existing preferences were persisted in snake_case format, they will be silently ignored and fields will use defaults, causing user settings to reset on upgrade. Add per-field#[serde(alias = "...")]attributes for legacy names to maintain compatibility:Backward-compat aliases
#[derive(Debug, Clone, Serialize, Deserialize, Type)] #[serde(default, rename_all = "camelCase")] pub struct AppPreferences { pub theme: String, + #[serde(alias = "quick_pane_shortcut")] pub quick_pane_shortcut: Option<String>, pub language: Option<String>, + #[serde(alias = "tasks_dir")] pub tasks_dir: Option<String>, + #[serde(alias = "areas_dir")] pub areas_dir: Option<String>, + #[serde(alias = "projects_dir")] pub projects_dir: Option<String>, pub ignore: Option<Vec<String>>, + #[serde(alias = "show_obsidian_features")] pub show_obsidian_features: Option<bool>, + #[serde(alias = "permanent_delete_tasks")] pub permanent_delete_tasks: Option<bool>, }
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@tdn-desktop/src/lib/deep-link-handler.ts`:
- Around line 280-307: Wrap the body-only update in the same mutation lifecycle
calls as other mutations: call markMutationStart(...) before invoking
commands.updateTask for the body (the branch that checks options.body and uses
newTask.id and options.body), and call markMutationComplete(...) in a finally
block after the await so the mutation marker is always cleared; keep the
existing success handling that updates queryClient.setQueryData for
vaultQueryKeys.task(newTask.id) and vaultQueryKeys.tasks() unchanged so cache
updates remain the same.
- Around line 200-210: handleOpenView currently casts the incoming view to NavId
without validation, which can route to an invalid state; update handleOpenView
to first validate that view is a known NavId (e.g., compare against the
enum/union of allowed NavId values or use a helper isValidNavId) before calling
useNavigationStore.getState().navigate; if view === 'no-area' keep existing
behavior, if validated call navigate({ type: 'nav', id: view as NavId }),
otherwise handle the unknown case by returning false or navigating to a safe
fallback (e.g., a default route) and ensure the function returns the appropriate
boolean.
In `@tdn-desktop/src/services/vault/init.ts`:
- Around line 136-138: The reinitialization path uses formatVaultError which
only returns a string, causing reinitializeVault to skip the logging/toast
behavior used by initializeVault; update the error handling in reinitializeVault
to call handleVaultError(result.error) (or invoke the same logging/toast helper
used by initializeVault) instead of formatVaultError so failures are logged and
a user-facing toast is shown consistently, then rethrow or propagate the
resulting Error as before.
- Around line 44-47: The call to commands.refreshVault() in the early return
path is not handling promise rejections; mirror the existing error-handling
pattern used for the other refreshVault call by either awaiting
commands.refreshVault() inside a try/catch or appending .catch(...) to log the
error and prevent unhandled promise rejections. Ensure you use the same
logger/error message style as the other refreshVault() invocation so failures
are recorded (e.g., wrap commands.refreshVault() in try { await
commands.refreshVault() } catch (err) { processLogger.error(...) } or
commands.refreshVault().catch(err => processLogger.error(...)) before
returning).
In `@tdn-desktop/src/services/vault/mutations.ts`:
- Around line 227-261: The create-project mutation (useCreateProject) never
calls markMutationStart(), so the file-watcher debounce isn't engaged; add a
call to markMutationStart() at the beginning of the mutationFn in
useCreateProject (before logger.debug/commands.createProject) so the debounce is
engaged for the duration of the mutation, keeping the existing
markMutationComplete() in onSuccess (and consider handling
markMutationComplete() on error if needed).
- Around line 40-71: The useCreateTask mutation is missing an onMutate that
calls markMutationStart(), so add an onMutate handler to useCreateTask which
invokes markMutationStart() before the mutation begins (mirroring
useUpdateTask/useDeleteTask), and ensure it returns any necessary context (or
undefined) so the file watcher is prevented from invalidating caches during the
create operation; keep existing onSuccess behavior that calls
markMutationComplete().
In `@tdn-desktop/src/services/vault/queries.ts`:
- Around line 31-33: Import the i18n instance (e.g. import i18n from 'i18next'
or the project's i18n entry) at the top of
tdn-desktop/src/services/vault/queries.ts and localize the operation labels
passed into handleVaultError by replacing raw strings like 'Loading tasks',
'Loading task', 'Loading projects', 'Loading project', 'Loading areas', and
'Loading area' with i18n.t('...') calls; update each throw new
Error(handleVaultError(..., '...')) invocation (the calls around result.status
checks that call handleVaultError) to pass i18n.t('Loading tasks') etc. so the
user-facing messages are translated.
🧹 Nitpick comments (2)
tdn-desktop/src/services/vault/init.ts (1)
5-9: Shadowing module-levelqueryClientimport with hook result.Line 31 declares
const queryClient = useQueryClient()which shadows the module-level import at line 9. While the hook version is correct for React context, this shadowing could cause confusion and potential bugs if someone adds code that inadvertently references the wrong one.Consider renaming the module-level import or the hook variable for clarity.
💡 Suggested fix
import { useQueryClient } from '@tanstack/react-query' import { listen } from '@tauri-apps/api/event' import { useEffect } from 'react' import { logger } from '@/lib/logger' -import { queryClient } from '@/lib/query-client' +import { queryClient as globalQueryClient } from '@/lib/query-client' import { commands, type AppPreferences } from '@/lib/tauri-bindings'And update line 126:
- queryClient.clear() + globalQueryClient.clear()Also applies to: 31-31
tdn-desktop/src/hooks/use-task-order.ts (1)
104-130: Consider optimizing the Zustand selector for keyed access.The current implementation subscribes to the entire
orderMap, causing re-renders when any key's order changes, not just the relevant one. For better performance, consider using a selector that extracts only the needed key's order:💡 Suggested optimization
export function createKeyedTaskOrderHook(config: KeyedTaskOrderConfig) { return function useKeyedTaskOrder(key: string, tasks: Task[]) { - // Get order map from Zustand (using selector syntax for performance) - const orderMap = useDisplayOrderStore(config.getOrderMap) - const storedOrder = orderMap?.[key] ?? null + // Get order for specific key from Zustand (using selector with shallow equality) + const storedOrder = useDisplayOrderStore( + state => config.getOrderMap(state)?.[key] ?? null + )Note: This may require
useShallowor a custom equality function if the array reference changes but content is the same.
…implify file age checks, and update documentation for clarity.
…ith error logging and return an empty vector on failure. Update documentation for ThreadPool error handling status.
…sition, path extraction, and ensure lock is released before file I/O. Update documentation to reflect completion status of related tasks.
…emoving unused parameter and updating related tests. Review and document the location of VaultDirs struct for clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tdn-desktop/src-tauri/src/commands/recovery.rs (1)
138-176: Don’t drop directory-entry errors silently.
entries.flatten()discards per-entry errors, which can hide filesystem issues and skip files without visibility. Consider logging and continuing.🐛 Suggested fix
- for entry in entries.flatten() { - let path = entry.path(); + for entry in entries { + let entry = match entry { + Ok(entry) => entry, + Err(e) => { + log::warn!("Failed to read recovery directory entry: {e}"); + continue; + } + }; + let path = entry.path();tdn-desktop/src/lib/bindings.ts (1)
373-410: Add#[serde(alias)]for each field or implement migration logic to support legacy snake_case preferences.The Rust structs
AppPreferencesandDummyVaultPathsuse#[serde(rename_all = "camelCase")]but lack field-level#[serde(alias)]attributes or migration logic. If existing on-disk preference files use snake_case keys, deserialization will fail silently or skip fields. The#[serde(default)]attribute only provides defaults for missing fields, not key name conversion. Before release, either:
- Add per-field aliases:
#[serde(alias = "quick_pane_shortcut")]for each field, or- Implement a migration step in
load_preferences_sync()to convert legacy formattdn-desktop/src/lib/date-utils.ts (1)
109-138: The timezone bug is real: date-only strings are parsed as UTC but extracted with local timezone methods.When
new Date('2025-06-15')is parsed, it creates UTC midnight. Extracting components withgetDate(),getMonth(),getFullYear()(which use local timezone) then reconstructing withnew Date(year, month, date)causes a mismatch in non-UTC timezones.For example, in UTC-5:
'2025-06-15'→2025-06-15T00:00:00Z(UTC) →2025-06-14 19:00(local) →getDate()returns14, not15- If today is
2025-06-15, a task due on2025-06-15would be classified as overdueThe test suite doesn't catch this because it runs with a mocked time in UTC. Both
isOverdue()andisToday()have the same vulnerability.Recommend: Parse date-only strings explicitly as local dates (e.g.,
new Date(year, month, date)directly from parsing the string components) or usedate-fnsconsistently throughout, which correctly handles local date semantics.
🤖 Fix all issues with AI agents
In `@tdn-desktop/src/lib/deep-link-handler.ts`:
- Around line 296-324: The body-update branch currently swallows failures from
commands.updateTask and still treats the operation as successful; wrap the await
commands.updateTask(...) call in a try/catch (or add error handling where
updateResult.status !== 'ok') and log the error (using the existing logging
facility or console) including context (newTask.id and options.body) so failures
are visible; ensure you still call markMutationStart() before the attempt and
only update queryClient via vaultQueryKeys.task and vaultQueryKeys.tasks() when
updateResult.status === 'ok', and consider returning/propagating a failure
indicator instead of unconditionally returning true.
- Around line 34-37: The hard-coded Sets validNavIds and validDevNavIds can
drift from the NavId and DevNavId type definitions; instead, export canonical
arrays (e.g., NAV_IDS and DEV_NAV_IDS) where NavId/DevNavId are declared and
import them into deep-link-handler.ts, then construct validNavIds = new
Set(NAV_IDS) and validDevNavIds = new Set(DEV_NAV_IDS); also add a unit test
asserting every value of NavId/DevNavId is present in those arrays to ensure
they stay synchronized.
🧹 Nitpick comments (3)
tdn-desktop/src/services/vault/init.ts (1)
57-61: Consider adding.catch()for consistency.The external change path handles
Result.errorvia.then(), but a promise rejection (e.g., network/IPC failure) would be unhandled. Consider adding.catch()for symmetry with the recent mutation path (lines 44-48).♻️ Suggested improvement
// Refresh the vault data from disk - commands.refreshVault().then(result => { - if (result.status === 'error') { - logger.error('Failed to refresh vault', { error: result.error }) - } - }) + commands.refreshVault() + .then(result => { + if (result.status === 'error') { + logger.error('Failed to refresh vault', { error: result.error }) + } + }) + .catch(err => { + logger.error('Failed to refresh vault', { error: err }) + })tdn-desktop/src-tauri/src/commands/recovery.rs (1)
125-136: Guard days-to-seconds conversion against overflow.
Even though the current constant is small, the helper is generic; a saturating multiply prevents wrap in release builds.♻️ Proposed change
- let cutoff_secs = now_secs.saturating_sub(days * 24 * 60 * 60); + let seconds_per_day = 24_u64 * 60 * 60; + let cutoff_secs = now_secs.saturating_sub(days.saturating_mul(seconds_per_day));tdn-desktop/src/lib/date-utils.ts (1)
70-103: Consider locale-specific short date ordering.The “further out” path still hard-codes month/day ordering. For locales that prefer day–month or different punctuation, a translation key (or locale formatter) would keep this aligned with i18n expectations.
♻️ Suggested change
- const monthName = t(monthKeys[monthIndex]) - return `${monthName} ${date.getDate()}` + const monthName = t(monthKeys[monthIndex]) + return t('dates.shortDate', { month: monthName, day: date.getDate() })You’d then add
dates.shortDateto locale files (e.g.,"{{month}} {{day}}"for English, and other locales can reorder).
… improve layout consistency
Introduce parseLocalDate function to correctly parse date-only strings as local dates, preventing timezone issues. Update formatShortDate, formatRelativeDate, isOverdue, and isToday functions to utilize the new parsing method. Improve error handling in vault initialization: Add catch block to log errors when refreshing vault data after external changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tdn-desktop/src/services/vault/init.ts`:
- Around line 56-71: The code calls commands.refreshVault() but does not await
its completion before calling queryClient.invalidateQueries(vaultQueryKeys.all),
causing refetches against stale data; change the flow to await the promise
returned by commands.refreshVault(), handle both Result status and thrown errors
(as currently done in the .then/.catch), and only call
queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all }) after
refreshVault resolves successfully (or after logging an error), referencing the
existing commands.refreshVault, the returned result.status/error handling, and
queryClient.invalidateQueries(vaultQueryKeys.all).
♻️ Duplicate comments (1)
tdn-desktop/src/services/vault/init.ts (1)
44-48: HandleResult.statusin the recent-mutation refresh path.Line 44 only handles rejections; a successful
Resultwithstatus: 'error'is still ignored. This is the same issue previously raised for this path. Consider mirroring the error-status check used later in the file.🔧 Proposed fix
- commands.refreshVault().catch(err => { - logger.error('Failed to refresh vault after recent mutation', { - error: err, - }) - }) + commands + .refreshVault() + .then(result => { + if (result.status === 'error') { + logger.error('Failed to refresh vault after recent mutation', { + error: result.error, + }) + } + }) + .catch(err => { + logger.error('Failed to refresh vault after recent mutation', { + error: err, + }) + })
🧹 Nitpick comments (2)
tdn-desktop/src/lib/date-utils.ts (1)
78-91: Consider simplifying the month index type assertion.The verbose union type cast spanning 13 lines could be simplified. Since
getMonth()always returns 0-11, a single-line assertion would be cleaner.✨ Suggested simplification
- const monthIndex = date.getMonth() as - | 0 - | 1 - | 2 - | 3 - | 4 - | 5 - | 6 - | 7 - | 8 - | 9 - | 10 - | 11 - const monthName = t(monthKeys[monthIndex]) + const monthName = t(monthKeys[date.getMonth() as keyof typeof monthKeys])Alternatively, if the explicit union is preferred for documentation purposes, keep it but consider extracting it as a type alias to reuse between
formatShortDateandformatRelativeDate.tdn-desktop/src/services/vault/init.ts (1)
162-173: Avoid order-sensitive comparison forignorearrays.
JSON.stringifymakes['a','b']and['b','a']look different. If order isn’t meaningful, this can trigger unnecessary reinitializations.♻️ Suggested refactor
export function vaultConfigChanged( oldPrefs: AppPreferences | undefined, newPrefs: AppPreferences ): boolean { if (!oldPrefs) return true + + const oldIgnore = [...(oldPrefs.ignore ?? [])].sort() + const newIgnore = [...(newPrefs.ignore ?? [])].sort() + const ignoreChanged = + oldIgnore.length !== newIgnore.length || + oldIgnore.some((value, index) => value !== newIgnore[index]) return ( oldPrefs.tasksDir !== newPrefs.tasksDir || oldPrefs.areasDir !== newPrefs.areasDir || oldPrefs.projectsDir !== newPrefs.projectsDir || - JSON.stringify(oldPrefs.ignore) !== JSON.stringify(newPrefs.ignore) + ignoreChanged ) }
| // Refresh the vault data from disk | ||
| commands | ||
| .refreshVault() | ||
| .then(result => { | ||
| if (result.status === 'error') { | ||
| logger.error('Failed to refresh vault', { error: result.error }) | ||
| } | ||
| }) | ||
| .catch(err => { | ||
| logger.error('Failed to refresh vault after external change', { | ||
| error: err, | ||
| }) | ||
| }) | ||
|
|
||
| // Invalidate all vault queries to trigger refetches | ||
| queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where refreshVault is defined in the codebase
echo "=== Searching for refreshVault definition ==="
rg -n "refreshVault|refresh_vault" --type ts --type tsx --type rs -g '!**/target/**' -g '!**/node_modules/**'Repository: dannysmith/taskdn
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Check the tauri-bindings directory structure
echo "=== Checking tauri-bindings ==="
find . -path "*/node_modules" -prune -o -path "*/target" -prune -o -type f \( -name "*tauri-bindings*" -o -name "*bindings*" \) -print | head -20Repository: dannysmith/taskdn
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Look for tauri command definitions and src-tauri setup
echo "=== Finding tauri command files ==="
fd -type f -name "*.rs" | grep -E "(command|vault)" | head -20Repository: dannysmith/taskdn
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Search for refreshVault using simpler rg syntax
echo "=== Searching for refreshVault ==="
rg "refreshVault|refresh_vault" -g '!**/target/**' -g '!**/node_modules/**'Repository: dannysmith/taskdn
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Examine the tauri-bindings file to understand refreshVault
echo "=== Examining tauri-bindings.ts ==="
cat -n ./tdn-desktop/src/lib/tauri-bindings.ts | head -100Repository: dannysmith/taskdn
Length of output: 1741
🏁 Script executed:
#!/bin/bash
# Look at the Rust bindings to see refreshVault implementation
echo "=== Examining Rust bindings.rs ==="
wc -l ./tdn-desktop/src-tauri/src/bindings.rsRepository: dannysmith/taskdn
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Examine the Rust refresh_vault implementation
echo "=== Rust refresh_vault implementation ==="
rg -A 20 "pub fn refresh_vault" tdn-desktop/src-tauri/src/commands/vault.rsRepository: dannysmith/taskdn
Length of output: 807
🏁 Script executed:
#!/bin/bash
# Check the full bindings.ts file to understand the Result wrapper
echo "=== Full bindings.ts to understand Result type and refreshVault ==="
cat -n ./tdn-desktop/src/lib/bindings.ts | grep -A 5 -B 5 "refreshVault"Repository: dannysmith/taskdn
Length of output: 568
🏁 Script executed:
#!/bin/bash
# Check if queryClient.invalidateQueries is awaited or async
echo "=== Checking invalidateQueries usage in the codebase ==="
rg -B 3 -A 3 "invalidateQueries" tdn-desktop/src/services/vault/init.tsRepository: dannysmith/taskdn
Length of output: 280
🏁 Script executed:
#!/bin/bash
# Check full context around lines 56-71 in init.ts to understand the event listener structure
echo "=== Full context of vault-changed listener ==="
sed -n '40,85p' tdn-desktop/src/services/vault/init.tsRepository: dannysmith/taskdn
Length of output: 1503
🏁 Script executed:
#!/bin/bash
# Check if there are other similar patterns in the codebase
echo "=== Other uses of refreshVault to understand intent ==="
rg -B 5 -A 10 "refreshVault" tdn-desktop/src/services/vault/init.tsRepository: dannysmith/taskdn
Length of output: 1112
🏁 Script executed:
#!/bin/bash
# Check what invalidateQueries returns - is it async?
echo "=== Checking queryClient.invalidateQueries type ==="
rg -B 5 -A 10 "invalidateQueries" tdn-desktop/src/services/vault/init.ts | head -30Repository: dannysmith/taskdn
Length of output: 403
🏁 Script executed:
#!/bin/bash
# Check TanStack React Query documentation or usage patterns in codebase
echo "=== Other invalidateQueries usages to understand if it's async ==="
rg "invalidateQueries" tdn-desktop/src -A 2 -B 2 | head -50Repository: dannysmith/taskdn
Length of output: 471
Await refreshVault() before invalidating queries to prevent stale data refetches.
invalidateQueries() executes synchronously right after refreshVault() returns its promise, before the Rust vault refresh actually completes. This causes queries to refetch against stale data. Await refreshVault() and check its Result status before invalidating.
🔧 Proposed fix
- // Refresh the vault data from disk
- commands
- .refreshVault()
- .then(result => {
- if (result.status === 'error') {
- logger.error('Failed to refresh vault', { error: result.error })
- }
- })
- .catch(err => {
- logger.error('Failed to refresh vault after external change', {
- error: err,
- })
- })
-
- // Invalidate all vault queries to trigger refetches
- queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all })
+ // Refresh the vault data from disk before invalidating queries
+ ;(async () => {
+ try {
+ const result = await commands.refreshVault()
+ if (result.status === 'error') {
+ logger.error('Failed to refresh vault', { error: result.error })
+ return
+ }
+ await queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all })
+ } catch (err) {
+ logger.error('Failed to refresh vault after external change', {
+ error: err,
+ })
+ }
+ })()📝 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.
| // Refresh the vault data from disk | |
| commands | |
| .refreshVault() | |
| .then(result => { | |
| if (result.status === 'error') { | |
| logger.error('Failed to refresh vault', { error: result.error }) | |
| } | |
| }) | |
| .catch(err => { | |
| logger.error('Failed to refresh vault after external change', { | |
| error: err, | |
| }) | |
| }) | |
| // Invalidate all vault queries to trigger refetches | |
| queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all }) | |
| // Refresh the vault data from disk before invalidating queries | |
| ;(async () => { | |
| try { | |
| const result = await commands.refreshVault() | |
| if (result.status === 'error') { | |
| logger.error('Failed to refresh vault', { error: result.error }) | |
| return | |
| } | |
| await queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all }) | |
| } catch (err) { | |
| logger.error('Failed to refresh vault after external change', { | |
| error: err, | |
| }) | |
| } | |
| })() |
🤖 Prompt for AI Agents
In `@tdn-desktop/src/services/vault/init.ts` around lines 56 - 71, The code calls
commands.refreshVault() but does not await its completion before calling
queryClient.invalidateQueries(vaultQueryKeys.all), causing refetches against
stale data; change the flow to await the promise returned by
commands.refreshVault(), handle both Result status and thrown errors (as
currently done in the .then/.catch), and only call
queryClient.invalidateQueries({ queryKey: vaultQueryKeys.all }) after
refreshVault resolves successfully (or after logging an error), referencing the
existing commands.refreshVault, the returned result.status/error handling, and
queryClient.invalidateQueries(vaultQueryKeys.all).
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.