Repository navigation
Refactor: Test suite hardening and introduction of React component testing - #72
Merged
Merged
Conversation
- Add jsdom + Testing Library vitest "component" project alongside the existing "node" project; add test:unit / test:component scripts and a jest-dom setup file. - Extract the duplicated "unseen latest features" gate into a pure getUnseenLatestFeatures shared by NewFeatureModal and page.tsx (no behavior change). - Component tests: StepCreateProject (icon a11y, disabled-while-submitting) and NewFeatureModal (boot gating, pagination, Skip All via forceOpen). - Unit tests: getUnseenLatestFeatures, createProject icon persistence + optimistic columns, tour/seen-feature UI actions, moveCard, getReadyCards, buildFolderTree, searchAll, formatRelative/getDueDateStatus. - E2E: What's New modal boot/Done/seen flows + onboarding wizard entry; ipc-mock gains an emittable listener bridge. - Document the two-project test setup in CONTRIBUTING.md; add v2.3.3 changelog.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplits Vitest into node and component projects, adds shared onboarding/search/Kanban helpers, wires them into app and component code, and expands unit, component, E2E, layout, utility, and IPC coverage. ChangesTest infrastructure, shared helper, and coverage updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (5)
src/store/slices/workspace.test.ts (1)
30-95: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd one test for the Electron replacement path.
These cases only lock down the optimistic node-only branch.
createProjectalso replaces the five placeholder columns with authoritative server rows afterwindow.electron.project.create(...); without a stubbed Electron-path test, regressions there will still pass while leaving client/server column ids out of sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/slices/workspace.test.ts` around lines 30 - 95, Add a test for the Electron path in createProject, since the current workspace tests only cover the optimistic node-only branch. In src/store/slices/workspace.test.ts, stub window.electron.project.create and assert that createProject uses the returned server project/columns instead of leaving the five placeholder columns in state, keeping client/server ids and column data in sync. Use the createProject flow and the existing projects/columns state assertions to verify the authoritative rows replace the placeholders after the Electron call.src/store/slices/ui.test.ts (1)
139-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the storage write too.
markFeatureAsSeen's contract includesstorage.set(SEEN_FEATURES_KEY, next), but these cases only verify in-memory state. Mocking storage here would catch regressions where seen-feature state stops surviving app restarts even though this suite still passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/slices/ui.test.ts` around lines 139 - 159, The markFeatureAsSeen tests only verify in-memory updates on seenFeatures and miss the storage contract. Update the ui.test.ts cases around setup() and markFeatureAsSeen to mock storage and assert storage.set(SEEN_FEATURES_KEY, next) is called with the expected array after each unique-id update, while preserving the idempotent/ordered behavior checks on the seenFeatures state.src/components/layout/NewFeatureModal.component.test.tsx (2)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecouple this suite from the live feature registry.
These assertions hard-code the current registry shape (
LATESTis last, latest version has one item, registry has at least four items). Every future feature release will make this suite fail for reasons unrelated to modal behavior. Prefer a local mocked registry/helper fixture so the test data stays stable.Also applies to: 42-54, 67-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/NewFeatureModal.component.test.tsx` around lines 26 - 29, The NewFeatureModal.component test suite is coupled to NEW_FEATURES_REGISTRY through the LATEST constant, so it will break whenever the live registry changes. Replace the direct registry dependency with a local mocked registry or fixture inside the test file and use that stable data for all assertions in the NewFeatureModal.component.test.tsx cases, including the scenarios currently covering the latest item and registry-length assumptions. Keep the test focused on modal behavior by referencing the local helper/fixture instead of the shared registry export.
82-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the Prev-button state explicitly.
The test name promises to verify that Prev is disabled on the first slide, but the body only proves that Next advances. A regression in the disabled state would still pass here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/NewFeatureModal.component.test.tsx` around lines 82 - 93, The test for NewFeatureModal currently only checks that clicking “Next Feature” advances to the second slide, but it does not verify the promised Prev-button disabled state on the first slide. Update the NewFeatureModal.component.test.tsx case to explicitly assert the Prev control is disabled when the modal opens on NEW_FEATURES_REGISTRY[0], then verify it becomes enabled after advancing with the “Next Feature” button and remains consistent with the slide state.src/lib/new-features-registry.test.ts (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an invariant test for
NEW_FEATURES_REGISTRYordering.This locks in “last entry wins,” but it still won’t fail if the real exported registry is accidentally appended out of order. Since both the modal and onboarding deferral now depend on that shared registry, add one test that asserts the actual
NEW_FEATURES_REGISTRYsatisfies the ordering contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/new-features-registry.test.ts` around lines 69 - 77, Add an invariant test for NEW_FEATURES_REGISTRY ordering: the current getUnseenLatestFeatures test only proves “last entry wins” for a local fixture, not the real exported registry. In src/lib/new-features-registry.test.ts, add a test that inspects NEW_FEATURES_REGISTRY directly and asserts its ordering contract (for example, that the latest feature is determined by the last item and the registry is not accidentally appended out of order). Use the existing getUnseenLatestFeatures and NEW_FEATURES_REGISTRY symbols to keep the check tied to the shared registry used by the modal and onboarding logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 664-677: Update the section intro so it matches the actual test
locations now described in the vitest project bullets. In the CONTRIBUTING.md
test guidance near the vitest config explanation, revise the lead-in text that
currently implies tests are only under electron/ so it also reflects src/ test
files, keeping it consistent with the `node` and `component` descriptions.
In `@src/components/notes/notes-view/buildFolderTree.test.ts`:
- Around line 78-85: The folder ordering in buildFolderTree is using
locale-dependent comparison, which makes the test and behavior
environment-sensitive. Update the sorting logic in buildFolderTree to use an
explicit, pinned collation (for example, a fixed locale and case-insensitive
options) when comparing folder names, or adjust the test only if the intended
contract is not deterministic ordering. Reference the buildFolderTree sorting
path and the folder name comparison used for the top-level folders.
In `@tests/e2e/smoke.test.ts`:
- Around line 365-378: The smoke test’s fresh-install override for
needsWorkspaceSetup is being applied too late via a timer, so the app can read
the default false before the mock is updated. Update the init flow in the smoke
test around addInitScript/buildIpcMock/STORE_ATTACH_SCRIPT so the override is
installed synchronously before navigation, either by folding it into the same
mock script or by letting buildIpcMock accept overrides for
window.electron.needsWorkspaceSetup.
---
Nitpick comments:
In `@src/components/layout/NewFeatureModal.component.test.tsx`:
- Around line 26-29: The NewFeatureModal.component test suite is coupled to
NEW_FEATURES_REGISTRY through the LATEST constant, so it will break whenever the
live registry changes. Replace the direct registry dependency with a local
mocked registry or fixture inside the test file and use that stable data for all
assertions in the NewFeatureModal.component.test.tsx cases, including the
scenarios currently covering the latest item and registry-length assumptions.
Keep the test focused on modal behavior by referencing the local helper/fixture
instead of the shared registry export.
- Around line 82-93: The test for NewFeatureModal currently only checks that
clicking “Next Feature” advances to the second slide, but it does not verify the
promised Prev-button disabled state on the first slide. Update the
NewFeatureModal.component.test.tsx case to explicitly assert the Prev control is
disabled when the modal opens on NEW_FEATURES_REGISTRY[0], then verify it
becomes enabled after advancing with the “Next Feature” button and remains
consistent with the slide state.
In `@src/lib/new-features-registry.test.ts`:
- Around line 69-77: Add an invariant test for NEW_FEATURES_REGISTRY ordering:
the current getUnseenLatestFeatures test only proves “last entry wins” for a
local fixture, not the real exported registry. In
src/lib/new-features-registry.test.ts, add a test that inspects
NEW_FEATURES_REGISTRY directly and asserts its ordering contract (for example,
that the latest feature is determined by the last item and the registry is not
accidentally appended out of order). Use the existing getUnseenLatestFeatures
and NEW_FEATURES_REGISTRY symbols to keep the check tied to the shared registry
used by the modal and onboarding logic.
In `@src/store/slices/ui.test.ts`:
- Around line 139-159: The markFeatureAsSeen tests only verify in-memory updates
on seenFeatures and miss the storage contract. Update the ui.test.ts cases
around setup() and markFeatureAsSeen to mock storage and assert
storage.set(SEEN_FEATURES_KEY, next) is called with the expected array after
each unique-id update, while preserving the idempotent/ordered behavior checks
on the seenFeatures state.
In `@src/store/slices/workspace.test.ts`:
- Around line 30-95: Add a test for the Electron path in createProject, since
the current workspace tests only cover the optimistic node-only branch. In
src/store/slices/workspace.test.ts, stub window.electron.project.create and
assert that createProject uses the returned server project/columns instead of
leaving the five placeholder columns in state, keeping client/server ids and
column data in sync. Use the createProject flow and the existing
projects/columns state assertions to verify the authoritative rows replace the
placeholders after the Electron call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7884962-c0e6-45c6-acf8-53192ad455ee
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
CONTRIBUTING.mdchangelogs/v2.3.3.mdpackage.jsonsrc/app/page.tsxsrc/components/layout/NewFeatureModal.component.test.tsxsrc/components/layout/NewFeatureModal.tsxsrc/components/notes/notes-view/buildFolderTree.test.tssrc/components/onboarding/StepCreateProject.component.test.tsxsrc/lib/new-features-registry.test.tssrc/lib/new-features-registry.tssrc/lib/utils.test.tssrc/store/slices/board.test.tssrc/store/slices/selectors.test.tssrc/store/slices/ui.test.tssrc/store/slices/workspace.test.tstests/e2e/smoke.test.tsvitest.config.tsvitest.setup.components.ts
- Introduce dynamic `--chrome-top` CSS variable to keep the chat panel correctly aligned with the top bar, accounting for the dynamic update banner height. - Update `NewFeatureModal.component.test.tsx` to use a local, stable feature registry fixture, preventing test brittleness when real release notes are modified. - Improve accessibility and UX in `NewFeatureModal` by adding an aria-label to the previous feature button and ensuring proper enabled/disabled states. - Update `CONTRIBUTING.md` to document the correct locations for test files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/notes/notes-view/buildFolderTree.ts`:
- Line 44: The top-level folder sorting in buildFolderTree is pinning collation
to English, which should be removed so names sort according to the runtime
locale. Update the topLevel.sort comparator to rely on the default locale
instead of passing "en", keeping the existing base sensitivity behavior. Use
buildFolderTree and the topLevel.sort call as the target spot for the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20e61c0b-88b2-4f55-a8eb-6dadfaffb0ae
📒 Files selected for processing (12)
CONTRIBUTING.mdsrc/app/page.tsxsrc/components/chat/UnifiedChatPanel.tsxsrc/components/layout/NewFeatureModal.component.test.tsxsrc/components/layout/NewFeatureModal.tsxsrc/components/layout/app-chrome.tsxsrc/components/notes/notes-view/buildFolderTree.tssrc/lib/new-features-registry.test.tssrc/store/slices/ui.test.tssrc/store/slices/workspace.test.tstests/e2e/smoke.test.tstests/fixtures/ipc-mock.ts
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- src/components/layout/app-chrome.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/layout/NewFeatureModal.tsx
- src/store/slices/ui.test.ts
- src/components/layout/NewFeatureModal.component.test.tsx
- tests/e2e/smoke.test.ts
- Prevent the "dark flash" on application startup by resolving theme colors and injecting them directly into the splash screen's initial CSS. - Update `MarkdownContent` to dynamically adjust text and component colors based on the `isUser` prop, ensuring content remains legible in user bubbles (accent-colored) versus standard grey-surfaced assistant bubbles.
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 (1)
src/components/chat/chat-panel/MarkdownContent.tsx (1)
113-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUser markdown is still only partially re-themed.
This
isUserbranch fixes headings/lists/quotes/inline code, buthr,tr,th, andtdlater in the samecomponentsmap still render assistant-surface tokens. User-authored tables will remain low-contrast on the accent bubble unless those renderers also branch onisUser.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/chat/chat-panel/MarkdownContent.tsx` around lines 113 - 147, The isUser theming in MarkdownContent is incomplete because the `components` map still leaves `hr`, `tr`, `th`, and `td` using assistant-surface styles, so user markdown tables and separators stay low-contrast. Update the existing `MarkdownContent` renderers for those symbols to branch on `isUser`, matching the same user/accent token approach already used for headings, lists, blockquotes, and inline code, so all markdown elements in user messages are consistently re-themed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/splash/bootsplash.ts`:
- Around line 82-89: The splash theme custom properties are being generated on
body, which prevents later updates from splash:theme on document.documentElement
from taking effect for descendants. Update buildSplashHtml so the generated
--splash-* declarations are emitted on :root/html instead of body, keeping the
initial palette and the runtime theme override aligned. Use the buildSplashHtml
initialVars generation and the splash:theme CSS/DOM update path as the key
symbols to adjust.
In `@src/components/chat/chat-panel/MarkdownContent.tsx`:
- Around line 103-111: The user-specific color branch in MarkdownContent should
stop using raw Tailwind whites and switch to CSS-variable tokens to match the
app’s color rules. Update the strongColor, headingColor, listColor, codeClass,
and quoteClass mappings so the isUser path uses token-based classes/values
instead of text-white, bg-white/20, border-white/40, and text-white/80, and
express any transparency with color-mix(in srgb, var(--token) X%, transparent).
Keep the non-user branch unchanged and use the existing token style already used
in this component as the reference.
---
Outside diff comments:
In `@src/components/chat/chat-panel/MarkdownContent.tsx`:
- Around line 113-147: The isUser theming in MarkdownContent is incomplete
because the `components` map still leaves `hr`, `tr`, `th`, and `td` using
assistant-surface styles, so user markdown tables and separators stay
low-contrast. Update the existing `MarkdownContent` renderers for those symbols
to branch on `isUser`, matching the same user/accent token approach already used
for headings, lists, blockquotes, and inline code, so all markdown elements in
user messages are consistently re-themed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f11b78d-ff18-41ea-8ae7-3358b4bab4a3
📒 Files selected for processing (4)
electron/splash/bootsplash.tssrc/components/agent/AgentMessageBubble.tsxsrc/components/chat/chat-panel/MarkdownContent.tsxsrc/components/notes/markdown-pipeline.bench.test.ts
Refactor the `isWriteChannel` logic to use a verb-based heuristic rather than an exhaustive denylist. This prevents silent re-hydration storms caused by new read channels defaulting to "write" (triggering full snapshots). Key changes: - Defined `READ_ACTIONS` set to identify read operations by suffix (e.g., "list", "get", "fetch"). - Added a `READ_CHANNELS` set for irregular channel names. - Updated `isWriteChannel` to prioritize these heuristics. - Added `electron/ipc/registry.test.ts` to verify classification and prevent regressions like `db:flow:url:fetch`. - Minor style fix: lifted `--initialVars` to `:root` in `bootsplash.ts` and added missing `--accent-fg` token.
Refactored the onboarding completion flow into a standalone `completeOnboarding` utility to improve maintainability and ensure consistent state management. Crucially, this change ensures that `hydrateFromElectron()` is explicitly awaited before dismissing the onboarding UI. This fixes a race condition where the main app could render with an empty store if the disk hydration had not finished before the onboarding component unmounted. Additional changes: - Added `src/lib/complete-onboarding.test.ts` to verify the hydration-before-exit requirement and confirm the logic for deferring tours based on unseen feature registry entries. - Added `src/components/onboarding/StepDone.component.test.tsx` to ensure the "Open Cairn" button correctly signals completion with the updated onboarding state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/complete-onboarding.test.ts`:
- Around line 43-68: The onboarding tests still only verify that hydrate was
invoked and that the overall function eventually resolves, not that hydration
finishes before onboarding is dismissed. Update the tests around
completeOnboarding and makeDeps so hydrateFromElectron returns a deferred
promise you control, then assert setOnboardingState(false) has not been called
before that promise resolves. Keep the ordering assertion in the
completeOnboarding path by checking the hydrate call settles before the
exit/dismiss action, using the existing completeOnboarding and
hydrateFromElectron symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a022e05-27ea-462e-9b32-1fc12e528038
📒 Files selected for processing (4)
src/app/page.tsxsrc/components/onboarding/StepDone.component.test.tsxsrc/lib/complete-onboarding.test.tssrc/lib/complete-onboarding.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/page.tsx
Refactor test utilities and cases in `complete-onboarding.test.ts` to strictly verify that `setOnboardingState` and other side effects are awaited after `hydrateFromElectron` completes. Previously, tests tracked the order of execution via an array of strings, which could be brittle. Replaced this with a promise-based control flow that uses `resolveHydrate` to explicitly pause execution and assert that onboarding is not dismissed while hydration is still in flight. This ensures that the application does not render against an empty store on the first run.
Adds comprehensive unit testing for core drag-and-drop utilities in both the Flow and Kanban modules to ensure consistent behavior for complex UI interactions. - `flow-utils.test.ts`: Validates group containment logic, including coordinate accumulation, smallest-enclosing-group prioritization, and absolute-to-relative coordinate conversion. - `board-dnd.test.ts`: Guards against regressions in Kanban drop-zone hit testing and card reordering math.
Task-list checkboxes inside markdown table cells now render as real clickable checkboxes (GFM emits none in table cells, so td/th renderers recover them) and toggle the source via a shared toggleCheckboxInSource helper that scans list-item and table-cell checkboxes in document order. The CodeMirror editor now wires insertNewlineContinueMarkup + deleteMarkupBackward into the keymap so ordered/bulleted lists auto-continue, exit cleanly from empty items, and renumber on Enter/Backspace. Adds 23 tests (note-editor-utils + headless-CodeMirror component test).
Owner
Author
|
@coderabbitai please re review we made some changes |
|
✅ Action performedFull review finished. |
|
✅ Action performedFull review finished. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
tests/e2e/smoke.test.ts (1)
289-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning the smoke test to the current latest feature ID/title.
These assertions are hard-coded to
v2.3.2-onboarding-tourand"Interactive Workspace Tour & Highlights", so every registry update will break the E2E suite even if the behavior is still correct. Derive the latest entry fromNEW_FEATURES_REGISTRY(or assert the modal more generically) to keep release-note churn out of the smoke tests.Also applies to: 314-318, 337-340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/smoke.test.ts` around lines 289 - 295, The smoke tests are hard-coded to the current latest feature ID/title, which makes them brittle whenever the registry changes. Update the affected boot/modal assertions in smoke.test.ts to derive the expected latest feature from NEW_FEATURES_REGISTRY, or make the checks generic enough to validate the modal behavior without naming a specific release entry. Apply this fix consistently in the latest-version smoke test cases around the bootFreshPage flow and the related modal/title assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/splash/bootsplash.ts`:
- Around line 316-345: The splash palette in the theme helper is out of sync
with the app tokens, so update the `light` and `dark` color maps in
`getSplashPalette` to match `src/app/globals.css` exactly. In particular, align
`success` and `error` with the current design tokens (including the light-theme
`error` value) so the splash screen uses the same colors as the main window.
In `@src/app/page.tsx`:
- Around line 333-340: `completeOnboarding()` is using a stale `seenFeatures`
snapshot from `useCairnStore.getState()` before `hydrateFromElectron()`
finishes, which can leave `pendingTutorial` outdated. Update
`completeOnboarding()` to read the latest `seenFeatures` only after hydration
completes, or change the call site in `src/app/page.tsx` to pass a getter
instead of the captured array; keep the logic around `setOnboardingState`,
`setPendingTutorial`, and `setTutorialActive` aligned with the refreshed state.
- Around line 350-355: The fixed chat-panel offset in the page chrome logic is
using a hard-coded pixel banner height, which can drift from the real `h-9` size
as font scaling changes. Update the `chromeTop` calculation in `page.tsx` to
derive the banner height from the same rem-based CSS dimension or a shared
custom property used by the update banner, rather than adding a fixed pixel
value. Keep the `updateBannerVisible` logic, but ensure the offset stays in sync
with the banner’s actual rendered height under `applyFontScale()` and the
`--font-scale` root sizing.
In `@src/components/agent/AgentMessageBubble.tsx`:
- Around line 254-257: The user message bubble wrapper in AgentMessageBubble
still uses a hardcoded text color, causing the bubble to mix raw colors with the
MarkdownContent user branch. Update the wrapper around MarkdownContent to use
the accent foreground CSS variable token instead of text-white, keeping all
user-bubble text colors consistent with the CSS-variable-only rule. Use the
AgentMessageBubble component and its MarkdownContent user branch as the
reference points when making the change.
In `@src/components/kanban/board-dnd.ts`:
- Around line 52-70: In board-dnd.ts, the no-op check in the card drop
resolution logic misses same-column drops onto the column body when the dragged
card is already the last card, so moveCard() still records a move. Update the
drop handling around the isOverColumn branch and the existing “No-op” guard to
also treat a same-column drop on the column container with targetIndex equal to
the current end position as unchanged, using the existing resolve logic in
board-dnd.ts and moveCard() call path to avoid timestamp/history updates when
nothing moved.
In `@src/components/kanban/column-utils.ts`:
- Around line 14-18: The parseWipLimit helper is accepting numeric prefixes
because it uses parseInt, which can truncate values like decimals or scientific
notation. Update parseWipLimit to validate that raw is a whole positive integer
string before converting it, and only return a number when the entire input
matches that format; otherwise return null. Keep the existing behavior for empty
input and invalid/non-positive values, and make sure the logic remains in
parseWipLimit so the WIP-limit dialog cannot save a different value than the
user entered.
In `@src/components/layout/sidebar-utils.ts`:
- Around line 13-23: The shortcut labels in buildShortcutMap are hardcoded to
the macOS symbol, but the shortcut handler in page.tsx accepts both metaKey and
ctrlKey. Update buildShortcutMap so it formats the label based on platform,
using ⌘ for macOS and Ctrl for Windows/Linux, and keep the existing map behavior
intact for the visibleNavItems entries.
- Around line 50-55: The due date helper in dueDateSeverity is misclassifying
earlier-today timestamps because Math.ceil can round negative partial days to 0,
causing overdue items to return "warning" instead of "danger". Update
dueDateSeverity to treat any timestamp earlier than now as overdue before
applying the 7-day warning check, and add a regression test in
sidebar-utils.test.ts that covers a due date from earlier today and expects
"danger".
In `@src/components/layout/sidebar.tsx`:
- Around line 419-427: The due-date calculation in sidebar.tsx is memoized only
on dueDate, so Date.now() never refreshes and the severity/diffDays values can
become stale while the app remains open. Update the logic around the useMemo in
the sidebar component to either compute these values on each render or drive the
memo from a changing now value/timer so dueDateSeverity, diffDays, and
dueDateLabel stay current.
In `@src/components/notes/markdown-editor.component.test.tsx`:
- Around line 35-55: The tests in markdown-editor.component.test.tsx are
bypassing the real CodeMirror keybinding path by calling
insertNewlineContinueMarkup and deleteMarkupBackward directly, so they do not
verify the EditorView keymap wiring. Update the helper functions used by the
relevant tests to dispatch actual Enter and Backspace keyboard events against
the EditorView/DOM target instead of invoking the commands, so the markdown
keymap ordering and defaultKeymap precedence are exercised through the real
bindings.
In `@src/components/notes/note-editor-utils.ts`:
- Around line 59-70: In note-editor-utils.ts, the checkbox index logic inside
the table-row branch of the line-toggling helper is counting every [ ]/[x] match
in a row, but renderCellWithCheckboxes only renders checkboxes that appear at
the start of a cell. Update the row scan to count and toggle only cell-leading
task tokens (the same pattern used by renderCellWithCheckboxes in
note-editor.tsx), so later literal checkbox text inside a cell does not affect
the source index or toggle the wrong token.
- Around line 47-53: The checkbox toggle logic in note-editor-utils should only
update the single list marker at the clicked position, not every checkbox token
on the line. In the marker-handling block that uses listLineRe and boxRe, change
the replacement so it targets only the matched checkbox at the list marker (for
example by using the match offset or a more specific pattern) instead of a
global line-wide replace. Keep the behavior inside the existing toggle flow that
sets changed and returns the updated line.
In `@src/components/search/search-utils.ts`:
- Around line 59-63: The merge logic in search-utils currently only seeds seen
with keyword ids, so repeated semantic hits can still be appended and create
duplicate rows in SearchPanel. Update the merge step that builds merged from
keyword and semantic so it also deduplicates semantic entries against each
other, using the existing seen set (or equivalent) to track ids as semantic
items are processed before pushing them.
---
Nitpick comments:
In `@tests/e2e/smoke.test.ts`:
- Around line 289-295: The smoke tests are hard-coded to the current latest
feature ID/title, which makes them brittle whenever the registry changes. Update
the affected boot/modal assertions in smoke.test.ts to derive the expected
latest feature from NEW_FEATURES_REGISTRY, or make the checks generic enough to
validate the modal behavior without naming a specific release entry. Apply this
fix consistently in the latest-version smoke test cases around the bootFreshPage
flow and the related modal/title assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88361e7d-8238-44da-8f67-6cf5065c6f86
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
CONTRIBUTING.mdchangelogs/v2.3.3.mdelectron/ipc/registry.test.tselectron/ipc/registry.tselectron/splash/bootsplash.tspackage.jsonsrc/app/globals.csssrc/app/page.tsxsrc/components/agent/AgentMessageBubble.tsxsrc/components/chat/UnifiedChatPanel.tsxsrc/components/chat/chat-panel/MarkdownContent.tsxsrc/components/flow/flow-utils.test.tssrc/components/kanban/board-dnd.test.tssrc/components/kanban/board-dnd.tssrc/components/kanban/board.tsxsrc/components/kanban/card-detail-utils.test.tssrc/components/kanban/card-detail-utils.tssrc/components/kanban/card-detail.tsxsrc/components/kanban/column-utils.test.tssrc/components/kanban/column-utils.tssrc/components/kanban/column.tsxsrc/components/layout/NewFeatureModal.component.test.tsxsrc/components/layout/NewFeatureModal.tsxsrc/components/layout/app-chrome.tsxsrc/components/layout/sidebar-utils.test.tssrc/components/layout/sidebar-utils.tssrc/components/layout/sidebar.tsxsrc/components/notes/ai-text-toolbar.component.test.tsxsrc/components/notes/markdown-editor.component.test.tsxsrc/components/notes/markdown-editor.tsxsrc/components/notes/markdown-pipeline.bench.test.tssrc/components/notes/note-editor-utils.test.tssrc/components/notes/note-editor-utils.tssrc/components/notes/note-editor.tsxsrc/components/notes/notes-view/buildFolderTree.test.tssrc/components/notes/notes-view/buildFolderTree.tssrc/components/onboarding/StepCreateProject.component.test.tsxsrc/components/onboarding/StepDone.component.test.tsxsrc/components/search/search-panel.tsxsrc/components/search/search-utils.test.tssrc/components/search/search-utils.tssrc/lib/complete-onboarding.test.tssrc/lib/complete-onboarding.tssrc/lib/new-features-registry.test.tssrc/lib/new-features-registry.tssrc/lib/utils.test.tssrc/store/slices/board.test.tssrc/store/slices/selectors.test.tssrc/store/slices/ui.test.tssrc/store/slices/workspace.test.tstests/e2e/smoke.test.tstests/fixtures/ipc-mock.tsvitest.config.tsvitest.setup.components.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/app/page.tsx (1)
350-359: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
--chrome-topon the same scale as the banner height.Line 355 adds a fixed
36px, butUpdateBannernow usesh-9, so its height follows the rootremsize. That makes the chat panel offset drift from the real chrome height as font scaling changes, and even at the current 14px root it no longer matches the rendered banner. Derive the banner contribution from2.25rem(or a shared CSS custom property) instead of hard-coding pixels.Proposed fix
- // occupies 41px (40px + 1px bottom border), plus the 36px (h-9) update banner + // occupies 41px (40px + 1px bottom border), plus the 2.25rem (h-9) update banner // when it's visible. The chat panel anchors to this so it never overlaps the // banner's download button and its header aligns with the Topbar. const updateBannerVisible = !!(updateVersion || updateDownloaded); - const chromeTop = 41 + (updateBannerVisible ? 36 : 0); + const chromeTop = updateBannerVisible ? "calc(41px + 2.25rem)" : "41px"; return ( <main className="flex flex-col h-dvh w-screen overflow-hidden bg-[var(--background)]" - style={{ "--chrome-top": `${chromeTop}px` } as React.CSSProperties} + style={{ "--chrome-top": chromeTop } as React.CSSProperties} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/page.tsx` around lines 350 - 359, The `chromeTop` calculation in `src/app/page.tsx` hard-codes the banner offset in pixels even though `UpdateBanner` uses `h-9` and scales with `rem`. Update the `chromeTop` logic in `page` so the banner contribution comes from the same scale as the banner height, either by using `2.25rem` or a shared CSS custom property, and keep the `--chrome-top` style value in sync with that shared measurement.src/lib/complete-onboarding.ts (1)
17-20: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't read a pre-hydration
seenFeaturessnapshot after the await.Line 44 evaluates
deps.seenFeaturesonly after Line 40 awaitshydrateFromElectron(), but this dependency is captured before hydration starts. If hydrate marks the latest release as already seen, this still computeshasUnseenLatestfrom stale data, which can leavependingTutorialtrue while the modal never opens and the opted-in tour never starts. Make this a getter (or otherwise re-read state after hydrate settles).Proposed fix
export interface CompleteOnboardingDeps { hydrateFromElectron: () => Promise<void>; setOnboardingState: (v: false) => void; setPendingTutorial: (v: boolean) => void; setTutorialActive: (v: boolean) => void; - seenFeatures: string[]; + getSeenFeatures: () => string[]; registry: NewFeature[]; } @@ if (!startTour) return; const hasUnseenLatest = - getUnseenLatestFeatures(deps.registry, deps.seenFeatures).length > 0; + getUnseenLatestFeatures(deps.registry, deps.getSeenFeatures()).length > 0;Also applies to: 43-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/complete-onboarding.ts` around lines 17 - 20, The onboarding flow in complete-onboarding.ts is reading a stale seenFeatures snapshot after hydrateFromElectron() completes, which can leave pendingTutorial out of sync with the latest state. Update the logic in completeOnboarding and its HasSeenFeatures dependency so seenFeatures is re-read after hydration settles, for example by making it a getter or otherwise fetching the current value when computing hasUnseenLatest and pendingTutorial.src/components/search/search-utils.ts (1)
59-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeduplicate semantic hits against each other before appending.
Line 62 only checks ids that came from
keyword, so repeated semantic hits for the same result still get pushed multiple times.SearchPanelbuilds one enriched row per semantic hit, so this still renders duplicate rows when several sections of the same note match.Suggested fix
export function mergeSemanticResults( keyword: SearchResult[], semantic: SearchResult[], ): SearchResult[] { const seen = new Set(keyword.map((r) => r.id)); const merged = [...keyword]; for (const s of semantic) { - if (!seen.has(s.id)) merged.push(s); + if (seen.has(s.id)) continue; + seen.add(s.id); + merged.push(s); } return merged; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/search/search-utils.ts` around lines 59 - 63, The merge logic in search-utils currently deduplicates semantic results only against keyword hits, so repeated semantic matches can still be appended multiple times. Update the dedupe in the function that builds the `merged` results to track semantic ids as well as keyword ids, and skip any semantic item whose id has already been seen before pushing it. Keep the change localized around the `seen`, `merged`, and semantic loop logic so `SearchPanel` receives one row per unique result.src/components/kanban/board-dnd.ts (1)
51-72: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat same-column drops onto the column body as no-ops when the card is already last.
This still misses the case where the dragged card is already last and
overIdis its own column, somoveCard()records a bogus update/history entry even though nothing moved. Please add the matching regression insrc/components/kanban/board-dnd.test.tsas well.Suggested fix
export function resolveCardDrop( columns: BoardColumn[], getColumnCards: (columnId: string) => TaskCard[], draggedCard: TaskCard, overId: string, ): { targetColumnId: string; targetIndex: number } | null { + const currentCards = getColumnCards(draggedCard.columnId); + const currentIndex = currentCards.findIndex((c) => c.id === draggedCard.id); let targetColumnId: string; let targetIndex: number; @@ - if ( - draggedCard.columnId === targetColumnId && - getColumnCards(targetColumnId)[targetIndex]?.id === draggedCard.id - ) { - return null; + if (draggedCard.columnId === targetColumnId) { + const normalizedTargetIndex = isOverColumn ? currentCards.length - 1 : targetIndex; + if (currentIndex === normalizedTargetIndex) return null; } return { targetColumnId, targetIndex }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/kanban/board-dnd.ts` around lines 51 - 72, The same-column column-body drop path in board-dnd.ts still records a move when the dragged card is already the last card in its column, so update the resolution logic in the drop handling around the draggedCard/targetColumnId/targetIndex check to treat that case as a no-op before calling moveCard(). Also add a regression in board-dnd.test.ts covering a card dropped onto its own column body while already last, and assert no move/history entry is created.src/components/kanban/column-utils.ts (1)
14-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject partial numeric strings instead of truncating them.
This is still accepting inputs like
1.5,1e2, and3 cardsas smaller integers, so the WIP-limit dialog can save a different value than the user entered. The new test insrc/components/kanban/column-utils.test.tsis also pinning that incorrect behavior.Suggested fix
export function parseWipLimit(raw: string): number | null { - if (raw.trim() === "") return null; - const n = parseInt(raw, 10); - if (isNaN(n) || n < 1) return null; + const trimmed = raw.trim(); + if (trimmed === "") return null; + const n = Number(trimmed); + if (!Number.isInteger(n) || n < 1) return null; return n; }#!/bin/bash node - <<'NODE' for (const value of ["1.5", "1e2", "3 cards", "5", " 12 "]) { const parsed = parseInt(value, 10); const numeric = Number(value.trim()); console.log({ value, parseInt: parsed, number: numeric, isInteger: Number.isInteger(numeric) }); } NODE rg -n '"3 cards"|parseWipLimit' src/components/kanban🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/kanban/column-utils.ts` around lines 14 - 18, parseWipLimit is truncating partial numeric inputs by using parseInt, so values like 1.5, 1e2, and 3 cards are being accepted as different integers than the user entered. Update parseWipLimit in column-utils to only accept fully valid whole-number strings, and return null for any input that contains extra characters or non-integer forms. Keep the existing trim/empty handling, but use a stricter validation path so the WIP-limit dialog only saves exact integers; also adjust the related expectations in column-utils.test to match the corrected behavior.src/components/notes/note-editor-utils.ts (1)
47-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the preview checkbox index mapped one-to-one with source tokens.
Line 51 still toggles every checkbox token on a task line, and Lines 61-69 still count every
[ ]/[x]in a table row. The preview only renders the list-marker checkbox and cell-leading table checkboxes, so a normal click can still flip the wrong persisted token once extra checkbox text appears on the same line or later in a cell.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/notes/note-editor-utils.ts` around lines 47 - 70, The checkbox mapping in note-editor-utils is still using every checkbox token on a line instead of only the preview-visible one. Update the toggle logic in the line-processing branch that handles list items and table rows so it matches the specific source token rendered by the preview, using the existing listLineRe, tableRowRe, and boxRe flow with seen/index to target only the first visible checkbox in each rendered location. Make sure a task line only toggles its marker checkbox and a table row only toggles the cell-leading checkbox that corresponds to the clicked preview token, without advancing or toggling extra checkbox tokens later in the same line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelogs/v2.3.3.md`:
- Around line 3-8: The release notes copy uses lowercase “markdown” in the
summary, but it should be capitalized consistently as “Markdown” in user-facing
text. Update the wording in the changelog entry so the summary and the bug-fix
description both use “Markdown,” keeping the rest of the release note content
unchanged.
In `@CONTRIBUTING.md`:
- Around line 670-675: The CONTRIBUTING.md description of the node test suite is
inaccurate: it should reflect the actual globs used by the Vitest node project,
not the broader pattern currently documented. Update the `node` suite entry to
match the selectors in `vitest.config.ts` (the `node`/`test:unit` project
coverage for `electron/**/*.test.ts`, `src/**/*.test.ts`, and
`src/**/*.test.tsx`), and keep the component suite text unchanged so
contributors know exactly which files are included.
In `@src/components/notes/note-editor.tsx`:
- Around line 426-435: The toggleCheckbox path in note-editor.tsx only updates
note.content, so derived plain-text state can get stale after preview checkbox
toggles. Update the same updateNote call in toggleCheckbox to also keep
contentText in sync with the new content, matching the behavior used elsewhere
in the note editor flow. Use the existing toggleCheckbox,
toggleCheckboxInSource, and updateNote symbols to locate the change.
In `@src/store/slices/ui.test.ts`:
- Around line 7-15: The shared storage spy is currently declared as a normal
const and then referenced inside the vi.mock() factory, but vi.mock() is hoisted
in Vitest and can evaluate before that initialization. Move the shared spy into
vi.hoisted() in ui.test.ts so the storage mock’s set implementation can safely
reference it during module evaluation, keeping the existing
storage.get/storage.set/storage.delete/storage.clear mock structure intact.
---
Duplicate comments:
In `@src/app/page.tsx`:
- Around line 350-359: The `chromeTop` calculation in `src/app/page.tsx`
hard-codes the banner offset in pixels even though `UpdateBanner` uses `h-9` and
scales with `rem`. Update the `chromeTop` logic in `page` so the banner
contribution comes from the same scale as the banner height, either by using
`2.25rem` or a shared CSS custom property, and keep the `--chrome-top` style
value in sync with that shared measurement.
In `@src/components/kanban/board-dnd.ts`:
- Around line 51-72: The same-column column-body drop path in board-dnd.ts still
records a move when the dragged card is already the last card in its column, so
update the resolution logic in the drop handling around the
draggedCard/targetColumnId/targetIndex check to treat that case as a no-op
before calling moveCard(). Also add a regression in board-dnd.test.ts covering a
card dropped onto its own column body while already last, and assert no
move/history entry is created.
In `@src/components/kanban/column-utils.ts`:
- Around line 14-18: parseWipLimit is truncating partial numeric inputs by using
parseInt, so values like 1.5, 1e2, and 3 cards are being accepted as different
integers than the user entered. Update parseWipLimit in column-utils to only
accept fully valid whole-number strings, and return null for any input that
contains extra characters or non-integer forms. Keep the existing trim/empty
handling, but use a stricter validation path so the WIP-limit dialog only saves
exact integers; also adjust the related expectations in column-utils.test to
match the corrected behavior.
In `@src/components/notes/note-editor-utils.ts`:
- Around line 47-70: The checkbox mapping in note-editor-utils is still using
every checkbox token on a line instead of only the preview-visible one. Update
the toggle logic in the line-processing branch that handles list items and table
rows so it matches the specific source token rendered by the preview, using the
existing listLineRe, tableRowRe, and boxRe flow with seen/index to target only
the first visible checkbox in each rendered location. Make sure a task line only
toggles its marker checkbox and a table row only toggles the cell-leading
checkbox that corresponds to the clicked preview token, without advancing or
toggling extra checkbox tokens later in the same line.
In `@src/components/search/search-utils.ts`:
- Around line 59-63: The merge logic in search-utils currently deduplicates
semantic results only against keyword hits, so repeated semantic matches can
still be appended multiple times. Update the dedupe in the function that builds
the `merged` results to track semantic ids as well as keyword ids, and skip any
semantic item whose id has already been seen before pushing it. Keep the change
localized around the `seen`, `merged`, and semantic loop logic so `SearchPanel`
receives one row per unique result.
In `@src/lib/complete-onboarding.ts`:
- Around line 17-20: The onboarding flow in complete-onboarding.ts is reading a
stale seenFeatures snapshot after hydrateFromElectron() completes, which can
leave pendingTutorial out of sync with the latest state. Update the logic in
completeOnboarding and its HasSeenFeatures dependency so seenFeatures is re-read
after hydration settles, for example by making it a getter or otherwise fetching
the current value when computing hasUnseenLatest and pendingTutorial.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88361e7d-8238-44da-8f67-6cf5065c6f86
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
CONTRIBUTING.mdchangelogs/v2.3.3.mdelectron/ipc/registry.test.tselectron/ipc/registry.tselectron/splash/bootsplash.tspackage.jsonsrc/app/globals.csssrc/app/page.tsxsrc/components/agent/AgentMessageBubble.tsxsrc/components/chat/UnifiedChatPanel.tsxsrc/components/chat/chat-panel/MarkdownContent.tsxsrc/components/flow/flow-utils.test.tssrc/components/kanban/board-dnd.test.tssrc/components/kanban/board-dnd.tssrc/components/kanban/board.tsxsrc/components/kanban/card-detail-utils.test.tssrc/components/kanban/card-detail-utils.tssrc/components/kanban/card-detail.tsxsrc/components/kanban/column-utils.test.tssrc/components/kanban/column-utils.tssrc/components/kanban/column.tsxsrc/components/layout/NewFeatureModal.component.test.tsxsrc/components/layout/NewFeatureModal.tsxsrc/components/layout/app-chrome.tsxsrc/components/layout/sidebar-utils.test.tssrc/components/layout/sidebar-utils.tssrc/components/layout/sidebar.tsxsrc/components/notes/ai-text-toolbar.component.test.tsxsrc/components/notes/markdown-editor.component.test.tsxsrc/components/notes/markdown-editor.tsxsrc/components/notes/markdown-pipeline.bench.test.tssrc/components/notes/note-editor-utils.test.tssrc/components/notes/note-editor-utils.tssrc/components/notes/note-editor.tsxsrc/components/notes/notes-view/buildFolderTree.test.tssrc/components/notes/notes-view/buildFolderTree.tssrc/components/onboarding/StepCreateProject.component.test.tsxsrc/components/onboarding/StepDone.component.test.tsxsrc/components/search/search-panel.tsxsrc/components/search/search-utils.test.tssrc/components/search/search-utils.tssrc/lib/complete-onboarding.test.tssrc/lib/complete-onboarding.tssrc/lib/new-features-registry.test.tssrc/lib/new-features-registry.tssrc/lib/utils.test.tssrc/store/slices/board.test.tssrc/store/slices/selectors.test.tssrc/store/slices/ui.test.tssrc/store/slices/workspace.test.tstests/e2e/smoke.test.tstests/fixtures/ipc-mock.tsvitest.config.tsvitest.setup.components.ts
…onboarding - splash: align success/error colors with app design tokens - onboarding: read seenFeatures via getter after hydration to avoid stale gate - page: derive chrome offset from rem-based banner height (font-scale safe) - agent: use --accent-fg token for user bubble text - kanban: no-op guard for last-card drop on own column body; strict integer WIP parsing - sidebar: platform-aware shortcut labels; overdue-first due-date severity; live now() in DueDateDot - notes: toggle only marker/cell-leading checkboxes; sync contentText on preview toggle - search: dedupe semantic results against each other - tests: real keymap events in markdown editor test; hoist ui storage spy; derive latest feature from registry in e2e; regression tests - docs: capitalize Markdown in changelog; correct node suite globs in CONTRIBUTING
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/components/search/search-utils.ts (1)
59-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeduplicate semantic hits against earlier semantic hits too.
seenis never updated inside the loop, so two semantic hits for the same result are both appended.SearchPanelrenders one row per merged result, so repeated note hits show duplicate rows.Suggested fix
export function mergeSemanticResults( keyword: SearchResult[], semantic: SearchResult[], ): SearchResult[] { - const seen = new Set(keyword.map((r) => r.id)); + const seen = new Set(keyword.map((r) => `${r.type}:${r.id}`)); const merged = [...keyword]; for (const s of semantic) { - if (!seen.has(s.id)) merged.push(s); + const key = `${s.type}:${s.id}`; + if (seen.has(key)) continue; + seen.add(key); + merged.push(s); } return merged; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/search/search-utils.ts` around lines 59 - 63, The merge logic in search-utils.ts only deduplicates semantic results against keyword results, because the seen set is never updated inside the semantic loop. Update the merging logic in the relevant function that builds merged search results so each appended semantic hit also gets recorded in seen, preventing duplicate semantic rows from reaching SearchPanel.src/components/kanban/column-utils.ts (1)
14-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject fractional/scientific values instead of truncating them.
Line 16 uses
parseInt, so inputs like"1.5"and"1e2"become1. That lets the dialog save a different WIP limit than the user entered.Suggested fix
export function parseWipLimit(raw: string): number | null { - if (raw.trim() === "") return null; - const n = parseInt(raw, 10); - if (isNaN(n) || n < 1) return null; + const trimmed = raw.trim(); + if (trimmed === "") return null; + const n = Number(trimmed); + if (!Number.isInteger(n) || n < 1) return null; return n; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/kanban/column-utils.ts` around lines 14 - 18, The parseWipLimit helper is truncating non-integer input because it uses parseInt, so values like fractional or scientific notation get converted to a different number than the user entered. Update parseWipLimit in column-utils.ts to validate the raw string as a plain positive integer before converting it, and keep returning null for any non-integer format; use the existing parseWipLimit symbol as the place to make the change.src/components/kanban/board-dnd.ts (1)
51-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat same-column drops onto the column body as no-ops.
If the dragged card is already last in its column, dropping it back on the column container resolves to
targetIndex === getColumnCards(overId).length, so the guard on Lines 67-70 misses the unchanged position.moveCard()then still rewritesupdatedAt, persists, and pushes a history entry.Suggested fix
export function resolveCardDrop( columns: BoardColumn[], getColumnCards: (columnId: string) => TaskCard[], draggedCard: TaskCard, overId: string, ): { targetColumnId: string; targetIndex: number } | null { + const currentCards = getColumnCards(draggedCard.columnId); + const currentIndex = currentCards.findIndex((c) => c.id === draggedCard.id); let targetColumnId: string; let targetIndex: number; @@ - if ( - draggedCard.columnId === targetColumnId && - getColumnCards(targetColumnId)[targetIndex]?.id === draggedCard.id - ) { - return null; + if (draggedCard.columnId === targetColumnId) { + const normalizedTargetIndex = isOverColumn ? currentCards.length - 1 : targetIndex; + if (currentIndex === normalizedTargetIndex) return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/kanban/board-dnd.ts` around lines 51 - 70, The same-column no-op check in board-dnd.ts is too narrow and misses drops onto the column container when the card is already last, causing moveCard() to still persist and record history. Update the drop-resolution logic around the targetColumnId/targetIndex handling so that the kanban drag handler treats a card dropped back onto its own column body at the end position as unchanged, and skip the move when the resolved slot matches the dragged card’s current position.
🧹 Nitpick comments (1)
tests/e2e/smoke.test.ts (1)
289-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected latest feature from the shared registry.
These assertions hardcode the current tail entry's title/id, so every future registry append will fail the smoke suite even when the gating behavior is still correct. Import the shared registry and compute the last entry once so the test follows the same contract as the app.
♻️ Suggested change
+import { NEW_FEATURES_REGISTRY } from "`@/lib/new-features-registry`"; + +const latestFeature = NEW_FEATURES_REGISTRY[NEW_FEATURES_REGISTRY.length - 1]; ... - await expect(p.getByText("Interactive Workspace Tour & Highlights")).toBeVisible(); + await expect(p.getByText(latestFeature.title)).toBeVisible(); ... - expect(seen).toContain("v2.3.2-onboarding-tour"); + expect(seen).toContain(latestFeature.id); ... - content: `localStorage.setItem("cairn:v1:seenFeatures", JSON.stringify(["v2.3.2-onboarding-tour"]));`, + content: `localStorage.setItem("cairn:v1:seenFeatures", JSON.stringify([${JSON.stringify("LATEST_FEATURE_ID")}]))`,Also applies to: 314-318, 340-340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/smoke.test.ts` around lines 289 - 295, The smoke test hardcodes the current latest-feature title instead of deriving it from the shared registry, so it will break whenever a new feature is appended. Update the affected expectations in smoke.test.ts to import the shared registry used by the app and compute the tail entry once, then assert against that entry’s title/id in the relevant tests so they stay aligned with the registry contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/globals.css`:
- Around line 18-21: The `--accent-fg` token in `globals.css` is too light for
the dark accent and causes the user-bubble text that uses it to fail AA at
`text-xs`. Update the dark theme foreground value to a darker color while
keeping white only for the light accent, and make sure the accent text tokens
remain consistent wherever `--accent-fg` is consumed.
In `@src/components/layout/sidebar.tsx`:
- Around line 419-425: The due-date calculation in the sidebar’s useMemo block
treats any partial future day as 1 day, so same-calendar-day deadlines can
render incorrectly. Update the diffDays logic in the sidebar component to first
detect when dueDate and now fall on the same calendar day and return 0 for that
case before applying the existing Math.ceil-based calculation, and add a
regression test covering a deadline later today rendering as “Due today.”
In `@src/lib/complete-onboarding.ts`:
- Around line 17-20: Avoid snapshotting seenFeatures before the awaited hydrate
completes; completeOnboarding should read the latest store state only after
hydrateFromElectron resolves, rather than relying on the pre-hydration array
passed from src/app/page.tsx. Update the call flow around completeOnboarding and
its deps.seenFeatures usage so the tutorial gating uses fresh seen-feature data
from the hydrated store, not stale pre-await state.
---
Duplicate comments:
In `@src/components/kanban/board-dnd.ts`:
- Around line 51-70: The same-column no-op check in board-dnd.ts is too narrow
and misses drops onto the column container when the card is already last,
causing moveCard() to still persist and record history. Update the
drop-resolution logic around the targetColumnId/targetIndex handling so that the
kanban drag handler treats a card dropped back onto its own column body at the
end position as unchanged, and skip the move when the resolved slot matches the
dragged card’s current position.
In `@src/components/kanban/column-utils.ts`:
- Around line 14-18: The parseWipLimit helper is truncating non-integer input
because it uses parseInt, so values like fractional or scientific notation get
converted to a different number than the user entered. Update parseWipLimit in
column-utils.ts to validate the raw string as a plain positive integer before
converting it, and keep returning null for any non-integer format; use the
existing parseWipLimit symbol as the place to make the change.
In `@src/components/search/search-utils.ts`:
- Around line 59-63: The merge logic in search-utils.ts only deduplicates
semantic results against keyword results, because the seen set is never updated
inside the semantic loop. Update the merging logic in the relevant function that
builds merged search results so each appended semantic hit also gets recorded in
seen, preventing duplicate semantic rows from reaching SearchPanel.
---
Nitpick comments:
In `@tests/e2e/smoke.test.ts`:
- Around line 289-295: The smoke test hardcodes the current latest-feature title
instead of deriving it from the shared registry, so it will break whenever a new
feature is appended. Update the affected expectations in smoke.test.ts to import
the shared registry used by the app and compute the tail entry once, then assert
against that entry’s title/id in the relevant tests so they stay aligned with
the registry contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88361e7d-8238-44da-8f67-6cf5065c6f86
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
CONTRIBUTING.mdchangelogs/v2.3.3.mdelectron/ipc/registry.test.tselectron/ipc/registry.tselectron/splash/bootsplash.tspackage.jsonsrc/app/globals.csssrc/app/page.tsxsrc/components/agent/AgentMessageBubble.tsxsrc/components/chat/UnifiedChatPanel.tsxsrc/components/chat/chat-panel/MarkdownContent.tsxsrc/components/flow/flow-utils.test.tssrc/components/kanban/board-dnd.test.tssrc/components/kanban/board-dnd.tssrc/components/kanban/board.tsxsrc/components/kanban/card-detail-utils.test.tssrc/components/kanban/card-detail-utils.tssrc/components/kanban/card-detail.tsxsrc/components/kanban/column-utils.test.tssrc/components/kanban/column-utils.tssrc/components/kanban/column.tsxsrc/components/layout/NewFeatureModal.component.test.tsxsrc/components/layout/NewFeatureModal.tsxsrc/components/layout/app-chrome.tsxsrc/components/layout/sidebar-utils.test.tssrc/components/layout/sidebar-utils.tssrc/components/layout/sidebar.tsxsrc/components/notes/ai-text-toolbar.component.test.tsxsrc/components/notes/markdown-editor.component.test.tsxsrc/components/notes/markdown-editor.tsxsrc/components/notes/markdown-pipeline.bench.test.tssrc/components/notes/note-editor-utils.test.tssrc/components/notes/note-editor-utils.tssrc/components/notes/note-editor.tsxsrc/components/notes/notes-view/buildFolderTree.test.tssrc/components/notes/notes-view/buildFolderTree.tssrc/components/onboarding/StepCreateProject.component.test.tsxsrc/components/onboarding/StepDone.component.test.tsxsrc/components/search/search-panel.tsxsrc/components/search/search-utils.test.tssrc/components/search/search-utils.tssrc/lib/complete-onboarding.test.tssrc/lib/complete-onboarding.tssrc/lib/new-features-registry.test.tssrc/lib/new-features-registry.tssrc/lib/utils.test.tssrc/store/slices/board.test.tssrc/store/slices/selectors.test.tssrc/store/slices/ui.test.tssrc/store/slices/workspace.test.tstests/e2e/smoke.test.tstests/fixtures/ipc-mock.tsvitest.config.tsvitest.setup.components.ts
- globals.css: darken dark-theme --accent-fg (#16082e) to pass WCAG AA on the #7c6af7 accent (was 3.99:1, now 4.75:1); light theme keeps white - sidebar: extract dueDateDiffDays helper that returns 0 for later-today deadlines (Math.ceil previously rounded up to 'Due in 1 day') - add regression tests for same-calendar-day and future-day diffs
Merged
5 of 17 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What does this PR do?
This PR introduces a dedicated component testing harness using
jsdomand@testing-library/reactto improve test coverage for the UI layer.Key changes include:
componentproject tovitest, allowing for JSDOM-based testing of React components.CONTRIBUTING.mdto distinguish betweennode(logic/store) andcomponent(UI/interaction) test suites, with new scripts (npm run test:unitandnpm run test:component).getUnseenLatestFeatures), removing code duplication between the modal and the app boot path.Type of change
Checklist
npm run type-check:allpassesnpm run lintpassesnpm testpassesvar(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)Notes for reviewer
This PR is an internal quality release infrastructure update. The added component tests provide much-needed coverage for complex UI interactions like the onboarding flow and feature-gating logic, which were previously difficult to verify in a standalone unit test environment.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation