Repository navigation
Conversation
WalkthroughWarning Review details and warnings were omitted to fit the comment limit. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis draft step establishes the package export and API location for service-specific test doubles, but does not yet implement the dependency-injected services or migrate existing global bridges, singletons, or tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ocavue
marked this pull request as ready for review
October 12, 2026 22:53
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
PR Summary by QodoReplace global TypeScript services with injected core and app layers
AI Description
Diagram
High-Level Assessment
Files changed (120)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 1143 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/services/browser-opener.ts">
<violation number="1" location="apps/desktop/src/services/browser-opener.ts:7">
P2: Browsers that drop transient activation across the clipboard await can block this popup, but `openUrl` still resolves and the GitHub flow shows no fallback URL. Keep the open in the click gesture or expose the verification link as a fallback.</violation>
</file>
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:51">
P3: This guidance does not apply to the extension's React components, which have no `useAppServices()` provider. Qualify it as desktop-specific so extension contributors are not directed to an unavailable hook.</violation>
</file>
<file name="apps/desktop/src/services/services-provider.tsx">
<violation number="1" location="apps/desktop/src/services/services-provider.tsx:41">
P3: This also throws beneath a valid provider when no graph session is open, so the error incorrectly blames missing provider context. Change it to identify the missing active graph.</violation>
</file>
<file name="apps/desktop/src/hooks/use-link-navigation-host.ts">
<violation number="1" location="apps/desktop/src/hooks/use-link-navigation-host.ts:30">
P2: A scope change away and back leaves the original key equal, so the old async activation can still navigate after its host context changed. Track a scope-change revision and compare the captured revision instead.</violation>
</file>
<file name="apps/desktop/src/mobile/classic-sign-in-progress.tsx">
<violation number="1" location="apps/desktop/src/mobile/classic-sign-in-progress.tsx:102">
P2: A transient keychain-read failure is logged, but `mutation.reset()` still closes the successful sign-in without publishing the new access. Keep this dialog open on read failure so the user can retry; `useClassicAccess` will not retry while its state has no token.</violation>
</file>
<file name="apps/desktop/src/services/browser-image-codec.ts">
<violation number="1" location="apps/desktop/src/services/browser-image-codec.ts:27">
P2: JPEG encoding drops transparency, so transparent areas in downscaled PNG or GIF attachments become black. Fill the canvas with an explicit background before drawing when `options.type` is JPEG.</violation>
</file>
<file name="apps/desktop/src/lib/shortcuts.ts">
<violation number="1" location="apps/desktop/src/lib/shortcuts.ts:40">
P2: This conversion silently drops earlier commands that share an exact binding before `KeymapService` can reject the duplicate. Build the record with an explicit duplicate check so conflicting app shortcuts fail during registration.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
This PR moves logic across many files. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
View guided diff | Turn on auto-fix | Re-trigger cubic
Code Review by Qodo
1. Credential read failures stall connection
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
 |
…o the `NoteRowService` overlay
# Conflicts: # packages/core/src/app/graph-session.ts
…add`, `RouterValue.navigationRevision` and test flush observers
Collaborator
Author
|
Thanks for the walkthroughs. The CodeRabbit, Sourcery and Greptile summaries describe the change and report no blocking finding, so there is nothing to act on from them. Every qodo and cubic inline thread has a reply and is resolved; the qodo summary findings without an inline thread are answered in a separate comment. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…he picker was open
…uring the contacts refresh
…ly while its graph is open
…hanged since the click
…rvice into service_r4_h12
…ore the spy after each test
…tead of counting digests
ocavuebot
changed the title
@reflect/core
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
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
2 participants
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.
Replaces the process-global IPC bridge and module-level singletons on the TypeScript side with dependency-injected services, and splits the TypeScript side into three layers:
@reflect/core(business logic and platform ports),@reflect/app(the app layer: windows, routes, commands, menus, update, theme, boot flows) andapps/desktop(adapters and React). No Rust logic changes (two test fixture paths moved).What changed
@reflect/corekeeps business logic and the interfaces of platform primitives; it knows nothing about windows, routes, menus, pixels, React or Tauri (lint-enforced: core cannot import@reflect/app, and theroleport is the only window fact core reads). New@reflect/app(packages/app) holds the app layer over core; it imports no React, TanStack or Tauri either, so it runs in theapp-nodevitest project.apps/desktopkeeps the platform adapters (apps/desktop/src/services/) and React.createCoreServices(adapters: CoreAdapters)(packages/core/src/create-core-services.ts) buildsCoreServicesandCoreInternalServices;createAppServices(core, host: AppHost)(packages/app/src/create-app-services.ts) buildsAppServices.apps/desktop/src/boot.tsruns them in order: the shared primitives (logger,clock, theIpcBridgeService), thencreateDesktopHost(window, lifecycle, operations and the other host-side services), then core with the host's services as its ports, then the app layer. Core layout: the two roots sit inpackages/core/src/, the host ports and their default implementations inpackages/core/src/ports/, everything else in a domain directory, and a file that exports a service is named after it. Pure computations are plain functions, not services:sha256Hexlives in@reflect/utils,pkceChallengeis called where it is used, and the mobile sync status is the functionmobileSyncStatus.ProgressPort(where visible background work is reported; the app'sOperationsServiceimplements it),ActivityPort(resume, focus, visibility and background-task signals; the app'sAppLifecycleServiceimplements it) androle: 'main' | 'note-window'.packages/core/src/ports/activity-port.tsandprogress-port.ts.ProgressPort.fail(label, message)is the one-shot form;start()returns a handle for long operations. A note window'sTaskServicefollows the Rustindex:writtenbroadcast itself throughipc.listen.XxxServiceDeps(readonly fields typed with the full service type; the deps interface lists only the services the class uses),XxxService(the documented contract), an unexportedXxxServiceImplclass (readonly #deps,#state, plain methods, no accessors, noprivate) andcreateXxxService(deps)returningnew XxxServiceImpl(deps). Files arepackages/<core|app>/src/<domain>/<name>-service.ts. OnlygetState/subscribeand the few members React passes unbound are function properties and arrow fields. Test doubles follow the same shape; a wrapper holds#innerand forwards each member, nothing spreads a service instance.createGraphServices(session, deps, flags)(core) starts backup, iCloud sync, capture, asset description, the chat session, iCloud downloads and the attachment catalog watch when a graph opens and disposes them whenGraphSession.signalaborts; it takes booleans and does no branching.createAppGraphServices(packages/app/src/graph/graph-lifecycles.ts) decides the flags from the window role, the platform and two live settings, and the desktop passes it to core throughCoreAdapters.createGraphServices.GraphSessionServiceis the state machine:attach,openRecent,createGraphAt,close,adopt, recents,deleteGraph,refreshIndex,park. AppGraphBootServiceholds the flows that know windows and dialogs:boot(main window, note window adoption, mobile),pickAndOpen,chooseGraph.IndexLifecycleService.followActivity()(core, started byGraphSessionService.attach()) suspends the index while a mobile main window is hidden.MobileOnboardingServiceis app layer.DeepLinkIntakeServiceowns thereflect://protocol: the OS source, the pending buffer,onUrl, andresolve(url)into a target (note, daily, search, tasks, capture spooled, none). AppDeepLinkRoutermaps a target to aRouteand navigates.Routeand the navigation history (NavigationService), link activation (LinkNavigationService), commands, keymap, native menus, update, theme, telemetry, the palette and autocomplete data layers, the calendar events store, the daily view models and the mobile keyboard and share services are all@reflect/app.NoteActionsService.deepLinkFor/absolutePathForreturn data and the desktop writes the clipboard (lib/clipboard-actions.ts);GistPublishService.publishreturns the URL;attachFilesToNotetakes the files the desktop picked.ClipboardService,OpenerServiceandDialogServiceare app-layer services (core has no caller left).generation. Rust-boundary wrappers (NoteFileService,AssetStoreService,IndexStoreService,GitRepositoryService, ...) take an explicitgeneration, reads included. User-facing services capture the openGraphSessiononce at each public method's entry (requireGraphSession,writeGenerationFor). Desktop code passes no generations and has no "No graph is open" guards.CoreServicesProvider/useCoreServices()andAppServicesProvider/useAppServices()(apps/desktop/src/services/services-provider.tsx);useServiceState(service, read)reads a service's state.useMaybeGraph(),useMaybeGraphServices()anduseMaybeChatSession()are the versions that returnnullwithout a graph.CommandContextcarriesservices(core) andapp. Providers build their context from the service state and the service; there is noSettingsProvider. Hook signatures (useGraph,useSettings,useSync,useChatSession,useRouter) did not change.TaskService.getState()/subscribe/refresh()and ten write methods, solib/tasks/keeps only UI orchestration), navigation and deep links, link navigation, capture, audio memos, asset descriptions, chat, embeddings and semantic search, backup and iCloud sync, calendar and contacts, monetization, the Classic access recheck policy, the per-graph lifecycles above.NoteRowServiceis the overlay for pin, pin order, private, gist and trash;NoteActionsServicewrites it and readers merge it. The app only invalidates React Query caches (apps/desktop/src/lib/query-invalidation.ts).setBridge/getBridge/hasBridge/useBridgeReady, every free function that delegated to a global, the desktoplib/modules that only forwarded calls,useMainWindowEffect, the desktop query-cache patches,GitHostCredentialService,IpcBridgeService.transport/hasPluginEvents,withAppReviewDemo/withAppReviewDemoTranscription,withContactsLookups, the*ServiceFactorylayers, the React Query task cache,packages/core/scripts/check-exports.mjs(a whole-repo check does not belong in lint; the barrels were trimmed by hand),eslint.architecture.js(folded intoeslint.config.js), the core browser vitest project and playwright.WikiLinkSuggestionServiceisAutocompleteSuggestionService;type Unlistenlives inpackages/core/src/unlisten.ts;PlatformFacts.buildPlatformcarries the Tauri build target; the core container isCoreServices(notAppServices, which is now the app layer's).Tests
@reflect/core/testingprovidescreateTestCoreServices(overrides)over the core root with test ports (createTestProgressPort(),createTestActivityPort()), plus hand-written doubles for the Rust, SQLite and plugin boundaries and recording wrappers over real services; where a real service's dependencies all have doubles, tests build the real service.@reflect/app/testingprovidescreateTestAppServices(core, overrides)over the app root with a test host (createTestAppHost()); its lifecycle double drives the same activity port the core services subscribe to, so oneemitResume()reaches both layers.renderInApp/renderHookInApp(apps/desktop/src/test-utils/render-in-app.tsx): both providers, the realGraphProviderand the real per-graph lifecycles over test services. Theapphandle is the core container,appServicesthe app layer.vitest-fail-on-consolecovers every package.restoreMocksis on.setBridge()vi.mock(callshasBridge()/useBridgeReady()call sitespackages/andapps/@reflect/coreThe remaining
vi.mockcalls are pure UI children, heavy children, Tauri adapter tests, and the browser extension. A lint rule holds the allowlist.Lint rules that keep the shape (
eslint.config.js)packages/corecannot import@tauri-apps/*,react,@tanstack/*,@reflect/app, or the apps;packages/appcannot import@tauri-apps/*,react,@tanstack/*, or the apps.isMainWindow,openNoteWindow,closeSecondaryWindowsor theRoutetype.default…/setDefault…instances or uses module-levellet.@tauri-apps/*is importable only fromapps/desktop/src/services/,boot.ts, andmain.tsx.consolein app or package source; logging goes throughLoggerService.vi.mockonly for an explicit allowlist of module specifiers.private/protected/publicor parameter properties (#instead).Behavior changes to review
The migration moved code and changed how dependencies are obtained. These are the places where behavior is not identical:
note:moved/ import-progress listener registration are now logged. Before, they were swallowed.UpdateService.supportedisfalseon iOS and Android. Before, it wastruein any native shell.hasBridge()/useBridgeReady()gates are gone. The IPC transport is installed before the first render, and the browser-dev transport queues early calls.applyChangesbatches (hash-gated).pinned: true.GraphSession.indexGenerationcan benull: a graph whose index failed to open still runs backup, iCloud sync, capture and chat, as before, and index-dependent actions no-op.noteDocuments.readSource,templates.templateBodyandbacklinks.getBacklinksWithContextthrow "No graph is open." without an open graph. The command palette note preview reads the open editor buffer first, then disk.onUrl) instead of a singlesetHandler; the first subscriber drains the buffer.master): asset-description re-indexing anddeepLinkForpassed the file generation where the index generation was expected, so those index writes could be rejected.packages/coregained adate-fnsdependency;packages/appdepends on@reflect/core,@reflect/utils,@ocavue/utils,date-fnsandzod.Fixes found on the way
asset-description-service.test.tsandnote-editor.test.tsxhad timing races;month-picker-drawer.test.tsxdepended on state left by another file.@tauri-apps/api/corewas optimized by Vite in the middle of a run, which reloaded the page. It is pre-optimized now.Not in this PR
master) are unchanged; the lines this PR adds are all multi-line.Docs
docs/contributing/services.md(with a "Core, app, or desktop?" section),testing.md,adding-a-command.md,adding-a-setting.md,editor-architecture.md,multi-window.md,deep-links.md,docs/plans/architecture-conventions.md,AGENTS.md,packages/core/README.md, and the newpackages/app/README.md.Written by Claude Fable 5.1, Claude Opus 5.5 and Claude Sonnet 5.5.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by Sourcery
Introduce replaceable core services for time, logging, identifiers, and hashing, and migrate affected application code and tests to use them.
New Features:
Enhancements:
Tests:
Chores: