Repository navigation
feat: YouTube-Wissenspipeline + STT-Arbeitsmodus - #22
Merged
Merged
Conversation
YouTube-Notizen-Pipeline mit LLM-Enrichment und Speech-to-Text-Arbeitsmodus mit YouTube-Kontext-Anreicherung. - Import: Google-Takeout-Watch-History, cookieless Transcript-Fetch, Note-Links aus dem Vault, Channel-Klassifikation (arbeit/privat) - Enrichment-Pipeline: mehrstufige LLM-Passes (Titel, Beschreibung, Zusammenfassung, Verwandt-Sektion), Timestamp-Linkify - Pass 5 ruft einen externen Such-Wrapper (OHS_WRAPPER_PATH) zur Cross-Vault-Wikilink-Disambiguierung auf (der Suchindex selbst liegt außerhalb dieses Repos) - Vault-Stub-Generierung mit Frontmatter (youtube_id) + Cross-Vault-Wikilinks - Health-/Morgen-Check (check-nightly-health) mit Failure-Tracking - youtube-context-lookup Skill + yt-lookup CLI - Speech-to-Text-Arbeitsmodus mit YouTube-Kontext (Eigennamen-Korrektur) - Kritische-Fehler- und Background-Status-Anzeige - yt-Schema + konsolidierte Init-Migration
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds SST tab mode and YouTube-binding flows, plus a new ChangesSST App: Tab Mode and YouTube Binding
yt-notes-scripts Package
Repo Tooling and Config
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 79
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/eslint-config/base.js (1)
21-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMirror the package-generated ignore here.
packages/*/src/generated/**is already excluded from Prettier, but this shared ESLint config still only ignoresapps/*/src/generated/**. Add the package glob too so generated Prisma output in packages doesn't get linted.Proposed fix
- ignores: ["dist/**", "apps/*/src/generated/**"], + ignores: ["dist/**", "apps/*/src/generated/**", "packages/*/src/generated/**"],🤖 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 `@packages/eslint-config/base.js` around lines 21 - 35, The shared ESLint ignore list in base.js only skips generated code under apps, so package-generated Prisma output is still being linted. Update the ignores entry in the ESLint config to include the packages/*/src/generated/** glob alongside the existing apps/*/src/generated/** pattern, keeping the change in the ignore block near the final config export.packages/yt-notes-scripts/src/utils/channel-classifier.test.ts (1)
1-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFlatten
describeblocks and usetest, per repo test-style guideline.No shared mock setup here to justify
describe, anditis used instead oftest.As per coding guidelines, "Use top-level
test()withoutdescribe()blocks; usedescribe()only when necessary (e.g., shared mock initialization)."🤖 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 `@packages/yt-notes-scripts/src/utils/channel-classifier.test.ts` around lines 1 - 55, The test file uses nested describe blocks and it, but the repo guideline prefers top-level test() without describe() unless shared setup is needed. Update the channel-classifier tests by flattening the suites in channel-classifier.test.ts, replacing describe/it with top-level test calls, and keep the same assertions for autoClassify and formatAutoNote.Source: Coding guidelines
apps/sst/src/data/text-improvement-actions.ts (1)
186-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading field name for Claude's model id, and unneeded remapping.
ImproveTextResult.ollamaModelIdnow also carries Claude'smodelIdin the work-mode branch (line 200), which will confuse anyone debugging correction quality by model. Also, the manual{ text: claudeResult.text, modelId: claudeResult.modelId }mapping (lines 187-190) is redundant sinceCorrectWithClaudeResultalready has that shape —correctionResult = await correctWithClaude({ prompt })would suffice.
ImproveTextResult'sollamaModelIdfield isn't shown in this batch — renaming it (e.g., tocorrectionModelId) may ripple into consumers of the tab-sync/UI response shape; please confirm before renaming.🤖 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 `@apps/sst/src/data/text-improvement-actions.ts` around lines 186 - 200, The correction result is being unnecessarily remapped and then stored in ImproveTextResult.ollamaModelId even when correctWithClaude is used, which makes the field misleading. In text-improvement-actions.ts, update the work-mode branch around correctWithClaude/correctionResult to assign the returned object directly instead of rebuilding { text, modelId }, and keep the current ImproveTextResult shape unless you have verified downstream consumers for a broader rename. Ensure the final result uses a clearly understood correction model source tied to the existing ImproveTextResult fields.apps/sst/src/routes/index.tsx (1)
1867-1917: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCritical/background error banner JSX is duplicated across both branches.
The same
hasCriticalError/hasBackgroundErrormarkup is rendered once inside thependingAction || statusMessagebranch (Line 1879-1899) and again, nearly identically, in theelsechain (Line 1900-1917). Extract a small helper (component or render function) to avoid the duplication and drift risk.🤖 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 `@apps/sst/src/routes/index.tsx` around lines 1867 - 1917, The critical/background error banner JSX is duplicated in the `pendingAction || statusMessage` branch and the fallback `else` chain, which makes the `hasCriticalError`/`hasBackgroundError` rendering easy to drift. Extract the repeated banner markup in `apps/sst/src/routes/index.tsx` into a small helper component or render function (for example, a reusable status banner near `statusMessage`/`pendingAction`) and call it from both branches so the `title`, error text, and conditional logic live in one place.
🤖 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 `@apps/sst/.env.base`:
- Around line 6-7: The .env.base entries for SHARED_VAULT_PATH and
YT_TEMPLATE_PATH need to match dotenv-linter expectations: remove the quotes
from their values and ensure the key order places SHARED_VAULT_PATH before
SST_OLLAMA_ENDPOINT. Update the relevant environment variable definitions in
this file so the ordering and formatting are consistent with the rest of the
dotenv settings.
In
`@apps/sst/prisma/migrations/20260607081744_add_tab_mode_and_youtube_id/migration.sql`:
- Around line 4-6: The tabs.migration adds mode as NOT NULL with a default in
one քայլ instead of the staged pattern required by the migration guideline.
Update this migration to follow the add-nullable → backfill → set-NOT-NULL flow
for mode, using the tabs table alteration and keeping youtube_id unchanged. Make
sure the final schema still enforces NOT NULL on mode after the backfill step.
In
`@apps/sst/prisma/migrations/20260608170154_add_tab_youtube_reused/migration.sql`:
- Around line 1-2: The tabs migration adds youtube_reused as NOT NULL with a
default in one step, which deviates from the staged migration guideline. Update
the migration for the tabs table to use the staged pattern: first add the
youtube_reused column as nullable, then backfill existing rows, and finally
alter it to NOT NULL; keep the same column name and migration flow consistent
with the other Prisma migration files.
In `@apps/sst/src/components/youtube-modal.tsx`:
- Around line 51-67: The submit flow in youtube-modal’s handleSubmit can bind a
stale video because it still prefers validationResult-based states after the URL
draft changes. Lock the URL input once validationResult becomes reusable or
ready_for_enrichment, or otherwise clear/reset validationResult when urlDraft
changes so onConfirmBind only uses the current validated URL. Update the related
submit logic and input state handling in youtube-modal to prevent binding an old
youtubeId after edits.
- Around line 69-141: The YouTube modal in youtube-modal.tsx is missing basic
keyboard accessibility, so update the modal wrapper in the YouTube modal
component to behave like an actual dialog. Add dialog semantics to the outer
modal container, wire an Escape-key handler to call onClose, and move initial
focus into the URL input when the modal opens. Keep the existing close/cancel
actions and ensure the input and buttons remain reachable and usable via
keyboard only.
In `@apps/sst/src/data/correct-with-claude.ts`:
- Around line 1-26: Add a test file for correctWithClaude to cover the wrapper
behavior in apps/sst/src/data/correct-with-claude.ts. Focus on the
correctWithClaude function: verify it trims whitespace from the callClaudeCli
result and that errors from callClaudeCli are propagated unchanged. Mirror the
style used by correction-prompts.test.ts so the critical correction path has
equivalent coverage.
- Around line 15-20: The `callClaudeCli` invocation in `correct-with-claude`
hardcodes the Claude `model` and `effort`, which should be configurable like the
sibling Ollama path in `text-improvement-actions`. Update this flow to read the
model/effort from environment-driven configuration loaded from `.env.base` and
`.env`, and fail fast if the required values are missing instead of using inline
defaults. Keep the existing `callClaudeCli` call structure, but wire `model` and
`effort` through the shared config pattern used elsewhere in the app.
- Around line 12-26: The Claude CLI invocation can hang indefinitely because
`correctWithClaude` awaits `callClaudeCli` with no timeout or abort path. Add a
timeout/fail-fast mechanism around the Claude call, either by extending
`callClaudeCli` in `llm-caller.ts` or wrapping the call in `correctWithClaude`,
and ensure the timeout is surfaced as a handled error so `improveTabRecordingFn`
doesn’t wait forever.
In `@apps/sst/src/data/correction-prompts.test.ts`:
- Around line 1-65: The test file groups cases with describe()/it() even though
there is no shared setup or mock initialization; refactor the tests in
buildPrivatePrompt and buildWorkPrompt to use flat top-level test() calls
instead of describe blocks. Keep the same assertions and inputs, but remove the
unnecessary nesting so the structure matches the coding guideline.
In `@apps/sst/src/data/correction-prompts.ts`:
- Around line 17-143: The prompt builders in buildPrivatePrompt and
buildWorkPrompt duplicate most of the same boilerplate, which is already
drifting between the two versions. Extract the shared intro, “AKTIV ERLAUBT”,
phonetic-correction rule, and “VERBOTEN” text into reusable constants/helpers,
then compose each prompt from that shared source while preserving the exact
rendered wording and ordering. Keep the work-specific YT-context priority block
and its numbering as the only variation, and use the existing
buildPrivatePrompt/buildWorkPrompt symbols to locate the refactor.
In `@apps/sst/src/data/tab-sync-actions.ts`:
- Around line 76-99: The enrichTabSnapshot helper is doing two ytPrisma lookups
for every server-only snapshot enrichment, which makes the text-sync path pay
extra latency on each write. Update enrichTabSnapshot to avoid re-querying on
every keystroke-driven action by adding a short-TTL cache keyed by
TabModel.youtubeId, or by limiting enrichment to lower-frequency flows such as
select/create. Keep the existing snapshot shape and the
toTabSnapshot/enrichTabSnapshot behavior, but ensure the transcript/video reads
are not repeated for every write-triggered call.
- Around line 227-244: The mode update branch in tab-sync-actions currently does
a separate prisma.tab.findUnique followed by prisma.tab.update, which can race
and still throw P2025 if the tab is deleted in between. Update the input.field
=== "mode" path in tabModeSchema handling to use an atomic update flow with the
same not_found result semantics as the other branches, or wrap the existence
check and update in a transaction so deletion races are handled safely.
In `@apps/sst/src/data/text-improvement-actions.ts`:
- Around line 169-191: Refactor the correction dispatch to use ts-pattern match
instead of the current if/else in the text improvement flow. In the logic that
builds correctionResult, replace the branching on tab.mode and tab.youtubeId
with a match-based expression that clearly handles the private/null case versus
the YouTube context case. Keep the existing behavior in buildPrivatePrompt,
loadYtContext, buildWorkPrompt, correctWithOllama, and correctWithClaude, but
move the case selection into a single match to follow the project convention.
In `@apps/sst/src/data/yt-binding-actions.test.ts`:
- Around line 16-137: The appendToNotizenSection tests are wrapped in a
redundant describe/it structure even though there is no shared per-test setup;
convert the cases to top-level test() calls and remove the describe() block.
Keep the existing assertions and test names, and use appendToNotizenSection as
the unique anchor while flattening each it() into its own standalone test().
In `@apps/sst/src/data/yt-binding-actions.ts`:
- Around line 20-21: The debug loggers in the yt binding actions module do not
follow the required app-prefixed naming and are created at module scope. Update
the debug logger setup around the yt binding actions functions to use the
`app:category:functionName` convention, create the logger at the start of each
function instead of as top-level constants, and use a consistent `debug`
variable name for the logging calls.
- Around line 108-120: The validateYoutubeUrlFn flow is mutating channel state
too early by updating the channel classification to "arbeit" inside the
validation/check step. Move that ytPrisma.channel.update out of the validation
path and defer it to bindYoutubeToTabFn, or perform it together with the bind in
the same transaction so the write only happens when the user actually confirms
the bind. Keep validateYoutubeUrlFn read-only aside from checks and returning
the validation result.
- Around line 276-279: The guard clauses in yt-binding-actions should not throw
raw Error objects for expected validation failures; instead, return structured
form-state errors like the existing { kind: "error", message } pattern used
elsewhere in the file. Update the affected action branches around the
tab-mode/youtubeId check, the missing-stub check, and the not-bound-to-video
check so the functions return error results consistently rather than throwing,
and keep the existing debugBind calls for tracing.
- Around line 271-279: The bind flow in bindYoutubeToTabFn has a TOCTOU race
because the work-mode/youtubeId guard is checked with findUniqueOrThrow and
later persisted with a separate tab.update, allowing concurrent calls to pass
together. Make the check-and-set atomic by moving the guard and update into one
transaction or conditional write around the tab lookup/update path, and use the
existing clientId in the lock/claim logic if that was intended. Ensure the
update branch in the enrichment path only proceeds after successfully claiming
the tab, so only one bind can run for a given tab at a time.
- Around line 25-33: The repo-root lookup in findRepoRoot currently starts from
process.cwd(), which can fail or crash at module load when the working directory
is outside the repository. Update findRepoRoot in yt-binding-actions to derive
the starting directory from import.meta.url using fileURLToPath and dirname,
then walk upward from there until turbo.json is found. Keep REPO_ROOT using the
revised findRepoRoot so repo discovery is stable regardless of cwd.
- Around line 464-473: The append-note flow in yt-binding-actions.ts ignores the
structured result from commitFile, so a failed git commit can still be followed
by tab deletion. Update the logic around the commitFile call to inspect its
{committed, reason} result and only proceed to sstPrisma.tab.delete after a
successful commit; if the commit fails, surface or handle the reason and stop
the deletion path. Use the existing commitFile invocation and the surrounding
appendToNotizenSection/writeFile flow to keep the note and tab state consistent.
- Around line 254-282: Both tab mutation handlers currently trust only tabId, so
any caller can bind or delete a tab without proving it belongs to the same
client/session. Update bindYoutubeToTabFn and pushNoteAndDeleteTabFn to verify
the supplied clientId (or equivalent server-side session identifier) before
performing any tab write/delete, and reject mismatches early. Use the existing
createServerFn handlers and the Tab lookup/deletion flow to add a consistent
ownership guard, since Tab does not yet carry an ownership field.
In `@apps/sst/src/data/yt-context.ts`:
- Line 25: The `yt-context` mapping is using a type assertion on
`video.transcript?.namedEntities` instead of validating the external JSON shape
at runtime. Replace the `as string[] | null` cast with Zod parsing/validation in
the code that builds the `namedEntities` value, and fall back safely when the
stored data is missing or invalid. Use the existing `yt-context` mapping path
around `namedEntities` and the downstream `buildWorkPrompt` flow to ensure only
a validated string array is passed through.
- Around line 10-20: The YT context lookup in loadYtContext() currently throws
on a missing row via findUniqueOrThrow, which aborts improveTabRecordingFn()
after transcription has already succeeded. Change loadYtContext() to handle the
missing metadata case gracefully by catching the lookup failure and returning a
safe fallback context (or otherwise triggering the private prompt/model path) so
the correction can continue instead of bubbling the error out.
In `@apps/sst/src/lib/title-truncation.test.ts`:
- Around line 1-24: The truncateTitle test file uses describe/it even though
there is no shared setup, so switch the suite in truncateTitle to top-level
test() calls. Keep the existing assertions intact, but replace the describe
block and nested it blocks with standalone test cases to match the testing
guideline and make the test file consistent.
In `@apps/sst/src/lib/title-truncation.ts`:
- Line 1: The module-level string constant ELLIPSIS should follow the
string-literal constant guideline by being declared with an explicit as const
annotation. Update the constant in title-truncation.ts so the identifier remains
ELLIPSIS in UPPER_SNAKE_CASE and is treated as a literal string type, matching
the module-level constant convention.
- Around line 3-6: The truncateTitle helper is slicing by UTF-16 code units,
which can split emoji or other astral characters and produce broken glyphs in
ytDisplayTitle. Update truncateTitle in title-truncation.ts to truncate by
grapheme cluster instead of using s.slice(0, maxLength), and keep the same
ELLIPSIS behavior for over-length titles. Use the truncateTitle symbol as the
single place to fix this so the index.tsx title rendering benefits
automatically.
In `@apps/sst/src/routes/index.tsx`:
- Around line 353-363: The `hasBackgroundError` and `hasCriticalError`
derivations in `index.tsx` are using chained literal comparisons against
`activeTab?.ytTranscriptStatus`; refactor this switch-like logic to use
`match()` from ts-pattern instead of multiple `||` checks. Keep the existing
status cases grouped under the same two booleans, and update the logic near the
`hasBackgroundError`/`hasCriticalError` declarations so the single source value
`ytTranscriptStatus` is matched cleanly and consistently with the project
guidelines.
- Around line 288-290: The current single-slot binding state in the index route
lets a second YouTube bind overwrite the first, so track in-flight binds per tab
instead of one global `bindingTabId`. Update the state and any related bind
start/end logic in `index.tsx` to key the spinner/lock by tab id, and make sure
the tab switch/add button gating uses the per-tab binding map so concurrent
binds cannot overlap.
- Around line 1252-1293: The success path in handleYoutubeConfirmBind can
overwrite a different currently active tab because applyActiveTab(result.tab)
always calls setActiveTab(nextTab). Update this flow to use the same guarded
pattern as updateServerTabSnapshot so the active tab is only replaced when the
resolved tab still matches the current active tab, and otherwise only update the
originating tab’s stored snapshot/state. Also remove the external-data type
assertion around bindYoutubeToTabFn by handling the returned value with proper
narrowing instead of an unconditional as cast.
- Around line 1236-1250: The Youtube validation handler is swallowing unexpected
failures and forcing them into modal error state instead of letting the route
boundary handle them. Update handleYoutubeValidate to only set
YoutubeValidationResult for expected { kind: "error" } responses from
validateYoutubeUrlFn, and rethrow or pass through unexpected thrown errors so
the route-level setRuntimeError can catch them. Also remove the unsafe
YoutubeValidationResult cast and add a runtime shape check on the
validateYoutubeUrlFn response before calling setYoutubeValidationResult.
In `@packages/yt-notes-scripts/package.json`:
- Around line 24-33: The package still relies on the `pg` import in
`migrate-from-main.ts`, so removing `@types/pg` will break TypeScript builds.
Either restore `@types/pg` in `packages/yt-notes-scripts/package.json`, or
update `migrate-from-main.ts` to remove the direct `Client` import from `pg` and
any related usage so the package no longer depends on those types.
In `@packages/yt-notes-scripts/prisma.config.ts`:
- Around line 29-31: The datasource URL construction in prisma.config.ts is
incorrectly appending ?schema= to DATABASE_URL, which can break existing query
parameters. Update the Prisma configuration so schema is handled outside the
connection string, and keep the connection setup in
packages/yt-notes-scripts/src/db.ts consistent with that approach. Use the
datasource config and db connection setup symbols to locate the related logic
and remove the redundant query-string concatenation.
In `@packages/yt-notes-scripts/README.md`:
- Around line 400-406: Resolve the inconsistency between the Scheduling and
Recovery-Workflow sections by choosing a single source of truth for
wrapper-script support. Update the README text around the Scheduling guidance
and the recovery steps that reference infra/launchd/yt-pipeline-nightly.sh so
both sections either state that no repo-shipped wrapper exists and users must
create their own, or clearly document that the wrapper is shipped and used by
the workflow. Make the wording consistent in the Scheduling section and the
Recovery-Workflow section so readers get one clear setup path.
- Around line 31-39: The README section describing “Hardcoded flags” is
inaccurate because `import_youtube_transcript.ts` does not always include
`--cookies-from-browser=chrome`; it is only added when `--with-chrome-cookies`
is passed. Update this doc block to match the actual `YT_DLP_FLAGS_BASE` /
optional-cookie behavior and the two-pass strategy documented later in the same
README, keeping the captions-only explanation aligned with the real
`import_youtube_transcript.ts` flow.
In `@packages/yt-notes-scripts/src/check-nightly-health.ts`:
- Around line 16-51: The logic in parseLastNightlyRun currently returns the
first matching end marker from the backward scan, which can miss a newer start
that is still hanging. Update parseLastNightlyRun to track the positions of the
last start and last end markers, then compare them so a later start without a
following end returns kind "hanging" instead of kind "ended". Use the existing
parseLastNightlyRun, endRegex, and startRegex flow to implement the index
comparison, and add a regression test for a log shaped like [start A][end
A][start B].
In `@packages/yt-notes-scripts/src/check-plugin-drift.ts`:
- Around line 6-9: Replace the promisified child_process exec usage in
check-plugin-drift.ts with execFile (or spawnSync per script guidelines) and
pass arguments as an array instead of building shell command strings from
baseline, head, and compareUrl. Update the call sites in readBaseline,
fetchHeadSha, and the compare/open logic to avoid shell interpolation entirely,
add a timeout for external process calls, and keep stdout/stderr capture
behavior equivalent. As defense-in-depth, validate the SHA values returned by
readBaseline and fetchHeadSha against a hex-SHA pattern before using them.
In `@packages/yt-notes-scripts/src/classify_channels.ts`:
- Around line 61-75: Phase A currently treats HttpError 403 like any other
non-404 and continues looping, causing repeated failing fetchVideo calls and
skip logs. Update the catch block in classify_channels.ts to detect 403 from
fetchVideo, abort the loop immediately (matching the break-on-403 behavior used
in get_video_details.ts refresh-stale), and keep the existing 404 unavailable
marking plus the generic skip handling for other errors.
In `@packages/yt-notes-scripts/src/cross-vault-link-validator.ts`:
- Around line 236-245: `buildVaultIndex` currently hides basename collisions by
keeping only the first lowercased basename it sees, which can make
`existsInShared` and `findInKb` resolve links to the wrong file. Update
`buildVaultIndex` to detect when the same key maps to multiple relative paths
instead of silently preserving the first match, and then handle that collision
explicitly in the lookup flow used by `findInKb`/`existsInShared` so ambiguous
basenames do not produce incorrect cross-vault links.
- Around line 262-280: formatRetryHint() currently hardcodes the shared-vault
name as test in multiple retry messages, so the guidance can be wrong when the
configured vault differs. Update this helper to accept the shared-vault name as
an argument and use it in all three message branches, then thread that value
through the call sites in cross-vault-link-validator.ts so the retry hint
matches the actual vault configuration.
In `@packages/yt-notes-scripts/src/db.ts`:
- Around line 21-24: The PrismaPg adapter setup in db.ts is redundantly
embedding the schema in the connection URL while also passing schema in the
second config argument. Update the adapter initialization to remove the ?schema=
query param from the DATABASE_URL string and keep the schema only in the
PrismaPg options (using YT_SCHEMA), so the connection string stays
schema-agnostic and follows the project guideline.
In `@packages/yt-notes-scripts/src/enrich-passes/pass1-audit.ts`:
- Around line 13-61: The prompt text for `buildPass1Prompt` and
`buildPass1ExtendedPrompt` is duplicated, so extract the shared
Werbung-erkennung/Sektionierung/timestamp rules into a common helper and reuse
it from both builders. Move the repeated template content into a shared function
like `buildSharedRules()` and interpolate it in both `buildPass1Prompt` and
`buildPass1ExtendedPrompt`, keeping only the input-specific parts separate.
Ensure the extracted helper preserves the exact existing prompt wording and
output structure so both paths stay in sync.
- Around line 63-71: The standalone runPass1 path is only used by tests, while
production uses runPass1Extended, so update pass1-audit to either remove
runPass1 entirely or clearly mark it as manual/standalone-only. If you keep it,
add explicit documentation near runPass1 and buildPass1Prompt explaining its
intended use; if not, delete the unused export and adjust any test references
accordingly.
In `@packages/yt-notes-scripts/src/enrich-passes/pass3-display-title.ts`:
- Around line 44-58: runPass3 currently returns the Claude-generated title
without enforcing the documented 80-character limit, so add a defensive check
after callClaudeCli and before returning the value. Use runPass3 and its
prompt/output flow to locate the fix, and either truncate or otherwise constrain
the cleaned title to 80 characters so the code enforces the contract even if the
model drifts.
In `@packages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.test.ts`:
- Around line 4-67: The test suite around buildPass5Prompt is using an
unnecessary describe wrapper with no shared setup or lifecycle hooks. Move the
existing top-level test cases out of describe and keep them as standalone test()
calls, preserving all assertions and test names. Use buildPass5Prompt as the
anchor when updating the structure.
In `@packages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.ts`:
- Around line 128-141: The runPass5 path currently grants the sub-agent
unrestricted Bash access, which is too broad for untrusted auditedMd input.
Update callClaudeCli in runPass5 to scope allowedTools to only the specific
wrapper command this pass needs, using the existing wrapper symbol (for example
OHS_WRAPPER_PATH) rather than a blanket Bash grant. Keep the change localized to
runPass5 and ensure the permitted tool pattern only allows that wrapper
invocation.
In `@packages/yt-notes-scripts/src/enrich-pipeline.counter.test.ts`:
- Around line 27-126: The updateAuditStatus counter transition tests are wrapped
in a describe block even though each case creates its own makeMockPrisma setup
and has no shared initialization. Remove the describe("updateAuditStatus —
counter transitions") wrapper and convert the six cases into top-level test()
calls, keeping the existing assertions and using updateAuditStatus and
makeMockPrisma as the key entry points.
In `@packages/yt-notes-scripts/src/enrich-pipeline.test.ts`:
- Around line 100-254: The shouldSkip tests are wrapped in an unnecessary
describe block even though there is no shared setup or mock state. Move the
individual tests in enrich-pipeline.test.ts out of the shouldSkip describe and
keep them as top-level test() cases, since describe() should only be used where
shared initialization like resetPassMocks is needed.
- Around line 528-565: The parallelization test in enrichVideoCritical is flaky
because it depends on real elapsed time from setTimeout-based mocks. Update the
test to verify concurrency via call ordering/state tracking on runPass3 and
runPass4 instead of asserting elapsed < 35ms; use the existing
pass3Started/pass4Started flags (or a resolvable-order check) to prove both
branches start before either completes, and remove the wall-clock threshold.
In `@packages/yt-notes-scripts/src/enrich-pipeline.ts`:
- Around line 135-145: The frontmatter reconstruction in enrich-pipeline.ts is
duplicated and uses ad-hoc regex/string splitting instead of the shared parsing
logic. Update the stub migration flow around stubPath/readFile/parseStub and the
later markdown rewrite path to call the same raw-frontmatter helper from
markdown-parser.ts (for example, a centralized getRawFrontmatterBlock-style
utility) so both paths rebuild frontmatter consistently. Remove the bespoke
md.split and /---/ extraction patterns and reuse the imported parser/helper
wherever the raw frontmatter block is needed.
In `@packages/yt-notes-scripts/src/enrich-status.ts`:
- Around line 71-86: The --since flag is currently declared in enrich-status but
never used, so the filter is ignored. Thread opts.since from program.action into
both showDefault() and showErrors(), and update those functions to accept and
apply the date filter in their Prisma queries. Make sure the ISO date value is
passed consistently through the reporting paths so the CLI flag actually changes
the output.
In `@packages/yt-notes-scripts/src/enrich-video.ts`:
- Around line 163-209: The enrich-video CLI action allows destructive bulk and
retry modes to run without any confirmation gate. Update the Command setup and
program.action in enrich-video.ts to add a -y/--yes flag for auto-confirmation
and, when it is not set, prompt with `@inquirer/prompts` confirm before executing
--bulk, --retry-errors, or --retry-post-critical-errors. Keep the default safe
(false) and only proceed into runMode after explicit confirmation.
- Around line 65-116: In retry-post-critical mode, the --limit currently applies
only after the database resets, so videos beyond the requested batch still get
their transcript state wiped without being reprocessed. Update enrich-video.ts
around the retry-post-critical branch so the bounded set is determined first,
then use that limited youtubeId list for both the
criticalErrors/backgroundErrors resets and the final videos array. Keep the
existing flow in sync with prisma.video.findMany, prisma.transcript.updateMany,
and the videos slicing logic so side effects only affect the same limited batch.
In `@packages/yt-notes-scripts/src/expand_legacy_links.ts`:
- Around line 13-16: The default template path in DEFAULT_TEMPLATE_PATH is
hard-coded to a developer-specific Dropbox location and will not work across
environments. Update expand_legacy_links.ts so the template location is read
from an environment variable such as YT_TEMPLATE_PATH, and make the
main/template resolution logic fail fast when neither that env var nor the
--template argument is provided. Use the existing DEFAULT_TEMPLATE_PATH constant
and the template loading flow in expand_legacy_links to locate and replace this
behavior.
In `@packages/yt-notes-scripts/src/git-commit-helper.test.ts`:
- Around line 33-38: Remove the unnecessary describe() wrappers around the git
commit helper tests and convert the cases in git-commit-helper.test.ts to
top-level test() calls. The shared setup/teardown already lives in the top-level
beforeEach() and afterEach(), so keep that and use the existing test names and
helpers like initRepo(), commitFile(), and commitAllInVault() directly without
grouping blocks.
In `@packages/yt-notes-scripts/src/git-commit-helper.ts`:
- Around line 1-36: The runGit helper can hang forever because the spawned git
process has no timeout, which can block the preRunCleanup/enrichment flow.
Update runGit to enforce a timeout, and if it expires, terminate the child
process and resolve with a timeout error; keep the fix localized to runGit and
its spawn/close/error handling.
In `@packages/yt-notes-scripts/src/import_youtube_history.ts`:
- Around line 116-124: The stub-upsert loop in import_youtube_history.ts is
doing a quadratic title lookup by calling processed.find(...) for every
youtubeId in the uniqueIds loop. Fix this by building a one-time lookup map from
youtubeId to title before the prisma.video.upsert loop, then read titles from
that map inside the loop while keeping the existing uniqueIds and upsert
behavior in import_youtube_history.ts.
In `@packages/yt-notes-scripts/src/import_youtube_transcript.ts`:
- Around line 36-56: The `runYtDlp` promise can hang forever when `yt-dlp`
cannot be spawned because only the `close` event is handled. Update `runYtDlp`
to listen for the child process `error` event and reject or resolve with a
failure so the promise always settles, then adjust `processTranscript` to catch
that rejection and treat it as a normal classified failure instead of an
uncaught exception, preserving the existing DB write and cleanup flow.
- Around line 94-100: The primary transcript selection in pickPrimaryTranscript
is non-deterministic when multiple .srt files exist, so update the logic to
choose a transcript by explicit language preference instead of the first
Array.find match. Use the CapturedTranscript metadata/filename in
pickPrimaryTranscript and the downstream import_youtube_transcript flow to
prefer de over en when both are present, and only fall back to the first
available transcript when neither preferred language is found.
In `@packages/yt-notes-scripts/src/llm-caller.ts`:
- Around line 111-113: The log in llm-caller’s request tracing currently prints
the full session identifier, which should be reduced for safety. Update the
console.log in llm-caller so the parsed.sessionId value is truncated or masked
before logging, while still preserving enough characters for diagnostics. Keep
the existing context fields (model, duration, turns, allowed_tools) and apply
the change at the sessionId formatting used in that log statement.
- Around line 76-124: The claude invocation in llm-caller.ts can hang forever
because the Promise returned around spawn("claude", args, ...) has no bounded
timeout. Add a timeout watchdog in the spawned process flow inside the Promise,
and on expiry terminate/kill the proc and reject with a timeout error so the
caller cannot remain pending indefinitely. Make sure the cleanup path clears the
timer on both proc.on("close") and proc.on("error") and keeps the existing
stdout/stderr handling intact.
In `@packages/yt-notes-scripts/src/markdown-parser.ts`:
- Around line 28-52: `findH2Sections` is treating `## ` inside fenced code
blocks as real headings, which breaks the section offsets used downstream.
Update the scanner in `markdown-parser.ts` to track fence state while iterating
lines, toggling when encountering a fenced code delimiter, and only start or
close an `H2Section` when not inside a fence. Keep the existing `findH2Sections`
shape and offset bookkeeping, but guard the heading detection so
`appendToNotizenSection` receives correct ranges.
In `@packages/yt-notes-scripts/src/recompute-plain-from-srt.test.ts`:
- Around line 51-58: The idempotency test in recomputePlainFromSrt is too weak
because `result.updated` is only checked with a non-negative assertion, which
can never fail. Update the test to verify the second run does not rewrite the
fixture row by asserting an exact expected value for `updated` (for example,
zero on the second invocation) and, if needed, compare the transcript row before
and after the second call using `prisma.transcript.findUniqueOrThrow` to ensure
no changes occurred.
In `@packages/yt-notes-scripts/src/recompute-plain-from-srt.ts`:
- Around line 24-31: The `recomputePlainFromSrt` candidate query currently
treats `opts.limit` as truthy, so `0` or a malformed value becomes “no limit”
and can trigger an unbounded run. Update the limit handling in
`recomputePlainFromSrt` to explicitly validate `opts.limit` before building the
Prisma query: reject invalid/NaN values and preserve an exact `0` instead of
dropping the `take` clause. Use the `recomputePlainFromSrt` and
`RecomputeOptions` symbols to locate the limit logic and make the behavior fail
fast for bad input.
In `@packages/yt-notes-scripts/src/relink-timestamps.ts`:
- Around line 222-224: The entrypoint in relink-timestamps should await the
asynchronous CLI parse so outer failures are handled correctly. Update the
import.meta.main block around program.parseAsync/process.argv to await the
promise and make sure any rejection from the findMany()/$disconnect() path is
surfaced as a clean non-zero exit, while leaving runForIds error handling
unchanged.
In `@packages/yt-notes-scripts/src/relocate_plugin_outputs.ts`:
- Around line 64-75: The obsidian CLI smoke test still builds a shell command
string in the `execSync` call, which should be switched to a safer
argument-based invocation for consistency and defense-in-depth. Update the
sanity check in `relocate_plugin_outputs.ts` to use the exec-file style API with
separate arguments instead of interpolating `sharedVaultName` into a shell
string, and keep the existing error handling and exit behavior in the smoke-test
block unchanged.
- Around line 439-511: The command in relocate_plugin_outputs.ts can run
destructive live actions immediately when --dry-run is omitted, without any
explicit confirmation gate. Update the Commander setup in program/.action to add
a -y/--yes auto-confirm flag and, when it is not provided, prompt for
confirmation with `@inquirer/prompts` before calling runLive; keep the default
safe by aborting unless the user explicitly confirms. Ensure the check is placed
around the live-execution path that follows runPlan, scanUserKbForCandidate, and
before runLive so accidental DB/file changes are prevented.
- Around line 383-402: The obsidian move command in relocate_plugin_outputs uses
execSync with a shell-interpolated string built from sourceVaultRelative and
targetVaultRelative, which allows command injection from DB-derived paths.
Replace the shell-string execution in relocate_plugin_outputs with a non-shell
invocation using an argument array or equivalent safe API so
noteLink.filePath-derived values are passed literally, and apply the same fix to
the smoke-test execSync call referenced in the file.
In `@packages/yt-notes-scripts/src/stub-creation.test.ts`:
- Around line 52-210: The test file repeats the same inline video mock shape
across many `createStubFile` cases, so extract a reusable factory helper (for
example near `describe("createStubFile")`) that builds the default video object
and accepts overrides. Update each `createStubFile` call to use that helper
instead of duplicating `youtubeId`, `title`, and `channel.name`, keeping special
cases like `publishedAt` and alternate titles as overrides only.
- Around line 52-99: Add a regression test in createStubFile that covers two
videos with the same sanitized title and channel but different youtubeId values,
and assert they do not overwrite each other. Use createStubFile, fakePrisma, and
the existing file/content assertions to verify each call produces a distinct
relPath/absPath and that both generated notes preserve their own video-specific
content. Also confirm the note_link upserts remain separate per youtubeId so the
collision case is caught if the filename logic regresses.
In `@packages/yt-notes-scripts/src/stub-creation.ts`:
- Around line 72-77: The renderTemplate helper in stub-creation.ts is silently
removing unknown placeholders by defaulting missing fields to an empty string.
Update renderTemplate so it does not mask typo’d or unsupported tokens in
user-editable templates; instead, either leave unknown placeholders unchanged or
raise a clear error when a key is missing. Keep the change localized to
renderTemplate and its template.replace handling so placeholder resolution is
explicit and easier to debug.
- Around line 32-67: The createStubFile flow is using only sanitized title and
channel name to build relPath, so different videos can collide on the same file
and the later noteLink upsert will incorrectly point a new youtubeId at existing
content. Update createStubFile to detect when an existing absPath already
belongs to a different video (for example by checking the stored noteLink for
the same vault/filePath or embedding the youtubeId in the filename/path), and
disambiguate the filename before writing and upserting. Keep the fix centered
around createStubFile, relPath/absPath construction, and the
prisma.noteLink.upsert call so each youtubeId maps to its own stub file.
In `@packages/yt-notes-scripts/src/stub-path-finder.ts`:
- Around line 23-29: `findStubPath` is selecting the first matching `noteLink`
row without a guaranteed DB order, so duplicate links can resolve
nondeterministically. Update the `prisma.noteLink.findMany` query in
`stub-path-finder.ts` to include an explicit `orderBy` that matches the intended
deterministic “first existing file” behavior, using the `findStubPath` logic and
`noteLink.findMany` result selection to ensure the same stub is chosen
consistently.
In `@packages/yt-notes-scripts/src/utils/file.ts`:
- Around line 21-32: The sanitizeFilename() helper can still return an empty
string after stripping illegal characters, trailing dots, and spaces, which
causes createStubFile() to build invalid paths like youtube/<channel>/.md.
Update sanitizeFilename() in file.ts to fall back to a safe placeholder name
whenever the sanitized result is empty, and make sure the path construction in
createStubFile() uses that non-empty value.
In `@packages/yt-notes-scripts/src/utils/obsidian-url-patcher.ts`:
- Around line 11-52: The matching in findUrlsToStub is too permissive because
indexOf(sourceUrl) can hit prefixes inside longer Obsidian links, such as Title
matching Title 2. Update the search logic in findUrlsToStub to only accept
matches that are followed by a valid URL boundary like ), &, whitespace, or
end-of-string, while still allowing &line= and similar query suffixes. Keep the
existing variant handling and dedup logic, but ensure each Patch is created only
for bounded matches.
In `@packages/yt-notes-scripts/src/utils/parser.test.ts`:
- Around line 7-100: The tests in parseVideoId, parseChapters, and
parseISODuration are only using describe() for grouping, which violates the test
convention here. Flatten the suite by moving each test.each/test case to
top-level test() calls in parser.test.ts, keeping the same assertions and
inputs, and preserve the existing unique symbols parseVideoId, parseChapters,
and parseISODuration for easy relocation.
In `@packages/yt-notes-scripts/src/utils/template.test.ts`:
- Around line 12-50: The template tests are wrapped in a unnecessary
describe("processTemplate") block, which goes against the test convention used
here. Flatten the suite by moving each test("...") to top level in
template.test.ts, and keep the existing processTemplate and SETTINGS assertions
unchanged since there is no shared setup to preserve.
In `@packages/yt-notes-scripts/src/utils/yt-marker.ts`:
- Around line 63-67: The PAREN_TS_REGEX in yt-marker.ts does not match the
documented “optional trailing whitespace” case, so Pass-5 timestamps like (3:24
) are missed. Update the regex used for parenthesized timestamps to allow
whitespace before the closing parenthesis, and then adjust the safeLinkify
replacement callback to preserve that whitespace in the output. If trailing
whitespace is not intended, remove that claim from the nearby comment so the
documentation matches the actual behavior.
In `@packages/yt-notes-scripts/src/yt-lookup.ts`:
- Around line 279-316: Wrap the DB lookup and output generation in
lookupAndPrint with error handling so documented exit code 3 can actually be
returned. Specifically, catch failures from prisma.transcript.findUnique and
formatOutput, log a clear yt-lookup error, and return 3 for DB-related errors
and invalid output-mode errors instead of letting them escape. Keep the existing
lookupAndPrint flow and handle the bare entrypoint’s try/finally by ensuring
these failures are converted into a normal numeric exit code.
- Around line 148-169: Replace the hand-rolled TranscriptRowForFormat and
VideoMetaForFormat interfaces with a Prisma-generated payload type derived from
the exact include used in yt-lookup; use the Prisma ModelGetPayload/validator
pattern so the return type is inferred from the query shape. Update the call
site that currently uses the double cast to rely on the generated type directly,
removing the as unknown as TranscriptRowForFormat bypass so schema changes
surface as TypeScript errors. Keep the type definition colocated with the query
logic in yt-lookup and reuse the same generated payload type anywhere the
formatted transcript row shape is referenced.
---
Outside diff comments:
In `@apps/sst/src/data/text-improvement-actions.ts`:
- Around line 186-200: The correction result is being unnecessarily remapped and
then stored in ImproveTextResult.ollamaModelId even when correctWithClaude is
used, which makes the field misleading. In text-improvement-actions.ts, update
the work-mode branch around correctWithClaude/correctionResult to assign the
returned object directly instead of rebuilding { text, modelId }, and keep the
current ImproveTextResult shape unless you have verified downstream consumers
for a broader rename. Ensure the final result uses a clearly understood
correction model source tied to the existing ImproveTextResult fields.
In `@apps/sst/src/routes/index.tsx`:
- Around line 1867-1917: The critical/background error banner JSX is duplicated
in the `pendingAction || statusMessage` branch and the fallback `else` chain,
which makes the `hasCriticalError`/`hasBackgroundError` rendering easy to drift.
Extract the repeated banner markup in `apps/sst/src/routes/index.tsx` into a
small helper component or render function (for example, a reusable status banner
near `statusMessage`/`pendingAction`) and call it from both branches so the
`title`, error text, and conditional logic live in one place.
In `@packages/eslint-config/base.js`:
- Around line 21-35: The shared ESLint ignore list in base.js only skips
generated code under apps, so package-generated Prisma output is still being
linted. Update the ignores entry in the ESLint config to include the
packages/*/src/generated/** glob alongside the existing apps/*/src/generated/**
pattern, keeping the change in the ignore block near the final config export.
In `@packages/yt-notes-scripts/src/utils/channel-classifier.test.ts`:
- Around line 1-55: The test file uses nested describe blocks and it, but the
repo guideline prefers top-level test() without describe() unless shared setup
is needed. Update the channel-classifier tests by flattening the suites in
channel-classifier.test.ts, replacing describe/it with top-level test calls, and
keep the same assertions for autoClassify and formatAutoNote.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3948bdc1-71e5-48b7-a8fe-02b18690bf55
⛔ Files ignored due to path filters (8)
apps/sst/src/generated/prisma/commonInputTypes.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/enums.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/class.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/prismaNamespace.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/prismaNamespaceBrowser.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/models/ModelRunLog.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/models/Tab.tsis excluded by!**/generated/**bun.lockis excluded by!**/*.lock
📒 Files selected for processing (115)
.gitignore.prettierignoreapps/sst/.env.baseapps/sst/package.jsonapps/sst/prisma/migrations/20260607081744_add_tab_mode_and_youtube_id/migration.sqlapps/sst/prisma/migrations/20260608170154_add_tab_youtube_reused/migration.sqlapps/sst/prisma/schema.prismaapps/sst/src/components/youtube-modal.tsxapps/sst/src/contracts/tab-sync.tsapps/sst/src/data/correct-with-claude.tsapps/sst/src/data/correction-prompts.test.tsapps/sst/src/data/correction-prompts.tsapps/sst/src/data/tab-sync-actions.tsapps/sst/src/data/text-improvement-actions.tsapps/sst/src/data/yt-binding-actions.test.tsapps/sst/src/data/yt-binding-actions.tsapps/sst/src/data/yt-context.tsapps/sst/src/lib/title-truncation.test.tsapps/sst/src/lib/title-truncation.tsapps/sst/src/routes/__root.tsxapps/sst/src/routes/index.tsxapps/sst/vite.config.tspackages/eslint-config/base.jspackages/yt-notes-scripts/.env.examplepackages/yt-notes-scripts/.gitignorepackages/yt-notes-scripts/.plugin-baseline.jsonpackages/yt-notes-scripts/.plugin-drift-log.mdpackages/yt-notes-scripts/README.mdpackages/yt-notes-scripts/bin/yt-marker-url.shpackages/yt-notes-scripts/docs/model-choice.mdpackages/yt-notes-scripts/package.jsonpackages/yt-notes-scripts/prisma.config.tspackages/yt-notes-scripts/prisma/migrations/20260616131000_init_squashed/migration.sqlpackages/yt-notes-scripts/prisma/migrations/20260630062703_add_video_unavailable/migration.sqlpackages/yt-notes-scripts/prisma/migrations/migration_lock.tomlpackages/yt-notes-scripts/prisma/schema.prismapackages/yt-notes-scripts/skills/youtube-context-lookup/SKILL.mdpackages/yt-notes-scripts/src/backfill-frontmatter-yt-id.test.tspackages/yt-notes-scripts/src/backfill-frontmatter-yt-id.tspackages/yt-notes-scripts/src/check-nightly-health.e2e.test.tspackages/yt-notes-scripts/src/check-nightly-health.test.tspackages/yt-notes-scripts/src/check-nightly-health.tspackages/yt-notes-scripts/src/check-plugin-drift.test.tspackages/yt-notes-scripts/src/check-plugin-drift.tspackages/yt-notes-scripts/src/classify_channels.tspackages/yt-notes-scripts/src/create_youtube_history.sqlpackages/yt-notes-scripts/src/create_youtube_note_links.sqlpackages/yt-notes-scripts/src/create_youtube_transcript.sqlpackages/yt-notes-scripts/src/create_youtube_video_details.sqlpackages/yt-notes-scripts/src/cross-vault-link-validator.test.tspackages/yt-notes-scripts/src/cross-vault-link-validator.tspackages/yt-notes-scripts/src/db.tspackages/yt-notes-scripts/src/enrich-passes/pass0-stub-migration.test.tspackages/yt-notes-scripts/src/enrich-passes/pass0-stub-migration.tspackages/yt-notes-scripts/src/enrich-passes/pass1-audit.test.tspackages/yt-notes-scripts/src/enrich-passes/pass1-audit.tspackages/yt-notes-scripts/src/enrich-passes/pass2-asr-fix.test.tspackages/yt-notes-scripts/src/enrich-passes/pass2-asr-fix.tspackages/yt-notes-scripts/src/enrich-passes/pass3-display-title.test.tspackages/yt-notes-scripts/src/enrich-passes/pass3-display-title.tspackages/yt-notes-scripts/src/enrich-passes/pass4-description.test.tspackages/yt-notes-scripts/src/enrich-passes/pass4-description.tspackages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.test.tspackages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.tspackages/yt-notes-scripts/src/enrich-pipeline.counter.test.tspackages/yt-notes-scripts/src/enrich-pipeline.test.tspackages/yt-notes-scripts/src/enrich-pipeline.tspackages/yt-notes-scripts/src/enrich-status.tspackages/yt-notes-scripts/src/enrich-video.tspackages/yt-notes-scripts/src/expand_legacy_links.tspackages/yt-notes-scripts/src/get_video_details.tspackages/yt-notes-scripts/src/git-commit-helper.test.tspackages/yt-notes-scripts/src/git-commit-helper.tspackages/yt-notes-scripts/src/import_youtube_history.tspackages/yt-notes-scripts/src/import_youtube_note_links.tspackages/yt-notes-scripts/src/import_youtube_transcript.tspackages/yt-notes-scripts/src/index-video.tspackages/yt-notes-scripts/src/index.tspackages/yt-notes-scripts/src/llm-caller.test.tspackages/yt-notes-scripts/src/llm-caller.tspackages/yt-notes-scripts/src/markdown-parser.test.tspackages/yt-notes-scripts/src/markdown-parser.tspackages/yt-notes-scripts/src/migrate-from-main.tspackages/yt-notes-scripts/src/pass5-sanitize.test.tspackages/yt-notes-scripts/src/pass5-sanitize.tspackages/yt-notes-scripts/src/recompute-plain-from-srt.test.tspackages/yt-notes-scripts/src/recompute-plain-from-srt.tspackages/yt-notes-scripts/src/relink-timestamps.tspackages/yt-notes-scripts/src/relocate_plugin_outputs.tspackages/yt-notes-scripts/src/stub-creation.test.tspackages/yt-notes-scripts/src/stub-creation.tspackages/yt-notes-scripts/src/stub-md-updater.test.tspackages/yt-notes-scripts/src/stub-md-updater.tspackages/yt-notes-scripts/src/stub-path-finder.tspackages/yt-notes-scripts/src/utils/channel-classifier.test.tspackages/yt-notes-scripts/src/utils/channel-classifier.tspackages/yt-notes-scripts/src/utils/file.test.tspackages/yt-notes-scripts/src/utils/file.tspackages/yt-notes-scripts/src/utils/note-link-detector.test.tspackages/yt-notes-scripts/src/utils/note-link-detector.tspackages/yt-notes-scripts/src/utils/obsidian-url-patcher.test.tspackages/yt-notes-scripts/src/utils/obsidian-url-patcher.tspackages/yt-notes-scripts/src/utils/obsidian-url.test.tspackages/yt-notes-scripts/src/utils/obsidian-url.tspackages/yt-notes-scripts/src/utils/parser.test.tspackages/yt-notes-scripts/src/utils/parser.tspackages/yt-notes-scripts/src/utils/relocator-planner.test.tspackages/yt-notes-scripts/src/utils/relocator-planner.tspackages/yt-notes-scripts/src/utils/template.test.tspackages/yt-notes-scripts/src/utils/template.tspackages/yt-notes-scripts/src/utils/yt-marker.test.tspackages/yt-notes-scripts/src/utils/yt-marker.tspackages/yt-notes-scripts/src/yt-lookup.test.tspackages/yt-notes-scripts/src/yt-lookup.tsturbo.json
💤 Files with no reviewable changes (5)
- packages/yt-notes-scripts/src/create_youtube_transcript.sql
- packages/yt-notes-scripts/src/create_youtube_video_details.sql
- packages/yt-notes-scripts/src/index.ts
- packages/yt-notes-scripts/src/create_youtube_note_links.sql
- packages/yt-notes-scripts/src/create_youtube_history.sql
The prisma.config.ts threw "DATABASE_URL is required" during `generate:prisma` in CI because it only loaded a gitignored `.env`, which does not exist on the runner. Mirror the mail-agent/box-storage pattern: add a committed `.env.base` with CI-safe DB defaults, load it before `.env` (override), and ignore `.env*` while allowing `.env.base`/`.env.example`.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
yt-notes-scripts tests import src/db.ts, which imports the gitignored generated Prisma client (src/generated/prisma/client). The turbo test task only depended on ^test, so on a fresh CI checkout the client did not exist when tests ran, failing with 'Cannot find module ./generated/prisma/client'. check-types already depends on generate:prisma; mirror that for test.
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 `@turbo.json`:
- Around line 53-57: The test task config is missing an explicit outputs
setting, which causes Turbo’s no-output warning for tasks that don’t produce
cacheable artifacts. Update the `test` task in `turbo.json` to include `outputs:
[]` alongside the existing `dependsOn`, `inputs`, and `outputLogs` fields so
Turbo treats it as an intentionally output-less task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
yt-notes-scripts build runs 'tsc -p', which type-checks against the gitignored generated Prisma client. Without generate:prisma as a dependency, a fresh CI checkout failed with TS2307 (Cannot find module ./generated/prisma/client) plus cascading implicit-any errors. box-storage (vite build) and mail-agent (no-op) were unaffected, so only this package broke. Mirror check-types/test.
CI has no Postgres and no DATABASE_URL. db.ts threw at import time, so any module importing it (even for the USER_KB_VAULT_NAME constant, or unit tests with a mocked DB like enrich-pipeline.test.ts) crashed on a fresh CI checkout. - db.ts: initialize the Prisma client lazily via a Proxy. Importing the module no longer throws; the DATABASE_URL check fires on first actual use (fail-fast preserved at the call site). - Rename the two tests that need a real database to *.e2e.ts so bun's default 'bun test' no longer auto-discovers them; add a 'test:db' script to run them locally against the docker-compose Postgres. No package runs DB-backed tests in CI, matching the existing convention.
The test asserted wall-clock elapsed < 35ms to distinguish parallel (~20ms) from sequential (~30ms) execution. That 15ms margin is too tight for slower, loaded CI runners, so it flaked. Assert the actual intent instead: Pass 4 must start while Pass 3 is still running. This is guaranteed by Promise.all's synchronous dispatch and independent of hardware timing, while still catching a regression to sequential awaiting (where Pass 3 finishes before Pass 4 starts).
migrate-from-main.ts imports Client from 'pg', which ships no type declarations. @types/pg was present in the local node_modules but undeclared and absent from the lockfile, so it resolved locally yet was missing on a fresh CI checkout (bun install --frozen-lockfile), failing the build with TS7016 'Could not find a declaration file for module pg'. Declare it as a devDependency and pin it in the lockfile.
Project test convention (AGENTS.md) uses top-level test() at the first level and reserves describe() for shared mock setup. Flattened purely-grouping describe wrappers across 8 test files and prefixed test names with the function under test. Kept the three describe blocks in enrich-pipeline.test.ts that own a beforeEach(resetPassMocks) for shared mock state. Addresses CodeRabbit review comments on PR #22.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
turbo.json (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill missing
outputs: []ontesttask.Previously flagged: the
testtask has nooutputsfield, triggering Turbo's "no output files found" warning. Not addressed in this revision.As per coding guidelines, "For packages with no build outputs, explicitly set
outputs: []in per-packageturbo.jsonto suppress Turbo warnings."♻️ Proposed fix
"test": { "dependsOn": ["generate:prisma", "^test"], "inputs": ["$TURBO_DEFAULT$"], + "outputs": [], "outputLogs": "new-only" },🤖 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 `@turbo.json` around lines 53 - 57, The turbo.json test task is still missing an explicit outputs declaration, which leaves Turbo thinking it may produce artifacts. Update the test task entry in turbo.json to include outputs set to an empty array alongside the existing dependsOn, inputs, and outputLogs settings so Turbo treats it as a no-output task and stops warning.Source: Coding guidelines
packages/yt-notes-scripts/src/db.ts (1)
32-35: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStill passing
?schema=in the connection string.Same issue flagged previously remains unresolved:
DATABASE_URLis suffixed with?schema=${YT_SCHEMA}while the secondPrismaPgargument already sets{ schema: YT_SCHEMA }. As per coding guidelines, "Do not use PostgreSQL schema parameter in connection URLs (e.g.,?schema=); useSET search_path TO schema_name;after connection instead."🔧 Proposed fix
const adapter = new PrismaPg( - { connectionString: `${DATABASE_URL}?schema=${YT_SCHEMA}` }, + { connectionString: DATABASE_URL }, { schema: YT_SCHEMA }, )🤖 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 `@packages/yt-notes-scripts/src/db.ts` around lines 32 - 35, The PrismaPg setup in db.ts still appends the PostgreSQL schema to DATABASE_URL, which duplicates the schema configuration already passed in the PrismaPg options. Update the PrismaPg initialization so it uses the plain DATABASE_URL without `?schema=...`, keep the `{ schema: YT_SCHEMA }` argument, and ensure any schema selection is handled through the connection/session setup rather than the URL.Source: Coding guidelines
🤖 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 `@packages/yt-notes-scripts/src/git-commit-helper.test.ts`:
- Around line 49-53: The test named commitFile no-op wenn Datei nicht dirty is
using a fresh empty repo, so it never covers the “tracked but clean” case.
Update git-commit-helper.test.ts to create and commit youtube/foo.md first, then
call commitFile(repo, "youtube/foo.md", "test commit") on the unchanged tracked
file and assert the no-op result. Use the commitFile helper and the existing
repo fixture setup to ensure the scenario is truly “already committed, unchanged
file” rather than a missing path.
---
Duplicate comments:
In `@packages/yt-notes-scripts/src/db.ts`:
- Around line 32-35: The PrismaPg setup in db.ts still appends the PostgreSQL
schema to DATABASE_URL, which duplicates the schema configuration already passed
in the PrismaPg options. Update the PrismaPg initialization so it uses the plain
DATABASE_URL without `?schema=...`, keep the `{ schema: YT_SCHEMA }` argument,
and ensure any schema selection is handled through the connection/session setup
rather than the URL.
In `@turbo.json`:
- Around line 53-57: The turbo.json test task is still missing an explicit
outputs declaration, which leaves Turbo thinking it may produce artifacts.
Update the test task entry in turbo.json to include outputs set to an empty
array alongside the existing dependsOn, inputs, and outputLogs settings so Turbo
treats it as a no-output task and stops warning.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ebe3b4a-2825-45a7-bcf2-dd372bc35c72
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
apps/sst/src/data/correction-prompts.test.tsapps/sst/src/data/yt-binding-actions.test.tsapps/sst/src/lib/title-truncation.test.tspackages/yt-notes-scripts/package.jsonpackages/yt-notes-scripts/src/check-nightly-health.e2e.tspackages/yt-notes-scripts/src/db.tspackages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.test.tspackages/yt-notes-scripts/src/enrich-pipeline.counter.test.tspackages/yt-notes-scripts/src/enrich-pipeline.test.tspackages/yt-notes-scripts/src/git-commit-helper.test.tspackages/yt-notes-scripts/src/recompute-plain-from-srt.e2e.tspackages/yt-notes-scripts/src/utils/parser.test.tspackages/yt-notes-scripts/src/utils/template.test.tsturbo.json
💤 Files with no reviewable changes (2)
- packages/yt-notes-scripts/src/check-nightly-health.e2e.ts
- packages/yt-notes-scripts/src/recompute-plain-from-srt.e2e.ts
Without a vitest.config, vitest inherited the app vite.config (tanstackStart, nitro, devtools, tailwind plugins) which kept watchers/servers open, so the Vite server never exited: 'close timed out after 10000ms' -> exit 1, turning 'bun run ci' red despite 5/5 green tests. The snapshots test is pure domain logic and needs none of that infra. Switch the test script to 'bun test' (matching box-storage / yt-notes-scripts) and port the vi.mock/vi.hoisted now() mock to bun's mock.module. Test task now exits 0.
…gates Replace shell-string exec with argument-array execFile so DB-derived vault paths and git refs can no longer break out into a shell: - relocate_plugin_outputs.ts: execSync -> execFileSync for the obsidian smoke-test and the vault move (vault/path/to values are array elements) - check-plugin-drift.ts: exec -> execFile for all gh/open call sites; add timeouts (baseline SHA from .plugin-baseline.json was the injection vector) Add confirmation gates for destructive defaults (commander -y/--yes, @inquirer/prompts confirm default false, non-TTY without --yes exits 1): - enrich-video.ts: --bulk / --retry-errors / --retry-post-critical-errors - relocate_plugin_outputs.ts: live run after the dry-run block Also: - enrich-video.ts: --limit now bounds the retry-post-critical reset via take: on both findMany queries, so updateMany no longer clears state on rows beyond the limit - classify_channels.ts: catch a 403 quota-exhausted HttpError and break the backfill loop instead of hammering guaranteed-failing API calls
Spawned child processes never settled their wrapping Promise when the binary
was missing (only close was handled), so a missing yt-dlp/git/claude hung the
run indefinitely:
- import_youtube_transcript.ts: add child.on("error", reject) to runYtDlp;
the call site catches spawn rejections, persists the reason to the
transcript row, and returns { ok: false } so the loop and workdir cleanup
continue. ENOENT maps to "yt-dlp not installed"
- git-commit-helper.ts: 30s timeout in runGit with a settled guard against
double-resolution; timer is unref'd
- llm-caller.ts: 10min timeout watchdog that kills the child and rejects;
clearTimeout in both the error and close handlers to avoid timer leaks
Also:
- import_youtube_transcript.ts: deterministic transcript language priority
(de before en, preferred-language SRT first) independent of filesystem
enumeration order
- llm-caller.ts: truncate session id to 8 chars in the diagnostic log
- git-commit-helper.test.ts: the clean-file no-op test now creates and commits
the file first, so it exercises the tracked-and-unchanged path instead of
the path-does-not-exist path
correct-with-claude.ts: - replace hardcoded model "sonnet" / effort "medium" with SST_CLAUDE_MODEL and SST_CLAUDE_EFFORT read via a Zod schema that fails fast on missing/invalid values, matching the SST_OLLAMA_MODEL_ID pattern in text-improvement-actions - add a 120s Promise.race timeout around the claude CLI call - add correct-with-claude.test.ts: trim, modelId, error propagation, and env validation (5 tests) declare the new env vars so the app starts and turbo caches correctly: - apps/sst/.env.base + .env.example (values preserve prior behavior) - turbo.json globalEnv also apply the linter conventions on the touched config files: - .env.base: drop value quotes and alphabetically order keys (dotenv-linter) - turbo.json: outputs: [] on the test task
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/sst/src/data/correct-with-claude.ts`:
- Around line 41-53: The timeout in correctWithClaude leaves its setTimeout
active after Promise.race resolves, so the later reject can trigger an unhandled
rejection. Update correctWithClaude to mirror the timer cleanup pattern used in
llm-caller.ts: keep the timeout handle, clear it when callPromise settles or
after the race completes, and ensure the timeout only rejects while the
operation is still pending. Use the correctWithClaude symbol to locate the race
setup and add cleanup around the timeoutPromise logic.
In `@packages/yt-notes-scripts/src/relocate_plugin_outputs.ts`:
- Around line 392-399: The obsidian subprocess calls in relocate_plugin_outputs
should be bounded like the git and gh commands. Update the execFileSync
invocation in the move flow and the smoke-test obsidian call to pass a timeout
option so a hung obsidian process cannot block the synchronous run indefinitely.
Use the existing execFileSync call sites in relocate_plugin_outputs to apply the
same timeout pattern consistently.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d1815b5-306b-46da-a54a-e76a1b272290
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
apps/financy-forecast/package.jsonapps/financy-forecast/src/domain/snapshots.test.tsapps/sst/.env.baseapps/sst/.env.exampleapps/sst/src/data/correct-with-claude.test.tsapps/sst/src/data/correct-with-claude.tspackages/yt-notes-scripts/package.jsonpackages/yt-notes-scripts/src/check-plugin-drift.tspackages/yt-notes-scripts/src/classify_channels.tspackages/yt-notes-scripts/src/enrich-video.tspackages/yt-notes-scripts/src/git-commit-helper.test.tspackages/yt-notes-scripts/src/git-commit-helper.tspackages/yt-notes-scripts/src/import_youtube_transcript.tspackages/yt-notes-scripts/src/llm-caller.tspackages/yt-notes-scripts/src/relocate_plugin_outputs.tsturbo.json
- obsidian-url-patcher.ts: findUrlsToStub now checks the character after each indexOf match against valid URL boundaries ()/&/whitespace/EOS). A prefix match such as searching "…/Title" hitting "…/Title%202" (file "Title 2") no longer produces a corrupt patched URL that silently redirects to a different note. Regression test added. - markdown-parser.ts: findH2Sections tracks fenced-code regions and skips "## " lines inside a ``` fence, so code examples cannot corrupt section offsets. New exported getRawFrontmatterBlock(md) helper. - enrich-pipeline.ts: both frontmatter extraction sites now call the shared getRawFrontmatterBlock helper instead of duplicated ad-hoc patterns; the regex form is also more robust than split(/\n---\n/) when YAML contains a literal \n---\n boundary. - file.ts: sanitizeFilename returns the replacement char for input that collapses entirely to dots/spaces (e.g. "...") instead of "", preventing hidden ".md" stub paths. Empty-string input still returns "". - yt-marker.ts: the timestamp regex now tolerates trailing whitespace before the closing paren, matching its documented intent.
…s, misc - pass5-summary-long.ts: replace the blanket allowedTools "Bash" grant with a scoped rule Bash(OHS_NODE_BIN=* <wrapper> *) so the summary sub-agent — whose prompt embeds untrusted transcript text — can only run the OHS wrapper it needs, not arbitrary shell. Compound-command payloads are blocked per sub-command and non-matching commands are denied in headless mode. - check-nightly-health.ts: parseLastNightlyRun does a single backward pass over start/end markers; a run started after the last completed one ([start A] [end A][start B]) is now reported as "hanging" instead of "ended". Regression test added. - relink-timestamps.ts: await program.parseAsync at the entrypoint so a rejection outside runForIds' try/catch yields a non-zero exit instead of a swallowed floating rejection. - import_youtube_history.ts: replace an O(n^2) title lookup with a Map built in one O(n) pass (first-match semantics preserved). - pass3-display-title.ts: warn when the produced title exceeds 80 chars (no downstream schema enforces it; hard truncation would silently corrupt titles). - pass1-audit.ts: document buildPass1Prompt/runPass1 as the standalone, manual-only non-extended path (production enriches via runPass1Extended).
…ncation - tab-sync-actions.ts: the mode-write branch used findUnique+update, which throws P2025 when the row is deleted between check and write. Use the updateMany + count-check + nullable findUnique pattern the other write branches already use, returning not_found instead. - title-truncation.ts: truncate by Unicode code points via Array.from so a multi-code-unit character (emoji) is never cut into a broken surrogate half; add as const to ELLIPSIS per convention. Test added for the emoji case.
- yt-binding-actions.ts: prefix debug loggers app: instead of sst:; derive the
repo root from import.meta.url instead of process.cwd(); move the unknown->
arbeit channel reclassification out of the validate dry-run into the confirmed
bind (before the critical enrich that re-reads the classification); return a
structured { kind: "error" } for the wrong-mode/already-bound guard instead of
throwing; throw on a real commitFile failure before deleting the tab so an
on-disk note is never orphaned
- index.tsx: track in-flight binds in a per-tab Set instead of a single slot, so
a second bind no longer overwrites the first and each bind clears only its own
tab; the success handler only updates the tab when it is still active
(updateServerTabSnapshot id-guard + activeTabIdRef-gated draft refresh);
derive hasBackgroundError/hasCriticalError via ts-pattern match(P.union);
route unexpected validation errors to setRuntimeError and drop an unchecked
cast
- yt-context.ts: parse the namedEntities Json field with a Zod schema instead of
an as-cast; use findUnique with a safe fallback instead of findUniqueOrThrow so
a missing metadata row no longer drops a completed transcription
- declare ts-pattern in apps/sst
…patch - youtube-modal.tsx: lock the URL input once a validation result resolves so the draft cannot diverge from the validated video before binding; add role="dialog", aria-modal, aria-labelledby, an Escape-to-close handler, and initial focus (focus trap left as a follow-up — no dialog primitive installed) - text-improvement-actions.ts: convert the correction dispatch to ts-pattern match over [mode, youtubeId], exhaustive over the finite union
- stub-creation.ts: leave unknown template placeholders verbatim instead of silently collapsing them to "" (typos in the user template stay visible) - stub-creation.test.ts: add a makeVideo factory and use it across the mocks - stub-path-finder.ts: add orderBy id asc so the stub path is selected deterministically when multiple rows match - cross-vault-link-validator.ts: track ambiguous basenames and surface a collision (warn + treat as unresolvable) instead of silently resolving to the first match; parameterize the shared-vault name in the retry hints
- enrich-status.ts: apply the declared --since filter to both report queries (it was parsed but never used) - recompute-plain-from-srt.ts: validate --limit (Number.isFinite), so NaN no longer silently disables the bound and --limit 0 means take 0 - recompute-plain-from-srt.e2e.ts: assert updated === 0 on the second run so the idempotency check can actually fail if recompute were non-idempotent - db.ts: drop the redundant ?schema= from the connection string (pg ignores it; the schema is set via the PrismaPg adapter arg) - prisma.config.ts: build the datasource URL with URL.searchParams.set so the schema param is not lost when DATABASE_URL already has query parameters - yt-lookup.ts: replace the hand-written row interface + double-cast with a Prisma select payload type; wrap the DB lookup so a DB error reaches the documented exit code 3
… path - README.md: the yt-dlp flag list and cookie flag now match the code (the cookie flag is opt-in, not hardcoded); remove the reference to a nightly wrapper script that is not shipped, pointing at the Scheduling section instead - expand_legacy_links.ts: replace the hardcoded personal Dropbox template path with process.env.YT_TEMPLATE_PATH (via --template), failing fast when neither is set; declare YT_TEMPLATE_PATH in the root turbo.json globalEnv
Two videos with the same sanitized title+channel but different youtubeId collided: the second skipped writing but still upserted its noteLink at the first video's filePath, so its DB record pointed at the wrong on-disk content. createStubFile now detects the collision (noteLink.findFirst on vault+baseRelPath with a different youtubeId) and only then switches to youtube/<channel>/<title>-<youtubeId>.md; a unique basename or a same-youtubeId re-run keeps the unsuffixed path, so existing files are not renamed. stub-path-finder reads filePath from the DB, so no change is needed there. Regression and idempotency tests added with a faithful findFirst/upsert mock.
Follow-ups from CodeRabbit's re-review of the new commits: - correct-with-claude.ts: clear the Promise.race timeout timer in a finally block so it cannot fire after the call resolves and reject an unawaited promise (unhandled rejection), matching llm-caller.ts - relocate_plugin_outputs.ts: add timeout 30s to both obsidian execFileSync calls, consistent with the git/gh subprocess timeouts
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 (5)
apps/sst/src/routes/index.tsx (2)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDebug namespace inconsistency: uses
sst:prefix, notapp:.
debugClient/debugClientButtonsare new module-scope loggers usingDebug("sst:yt:client")/Debug("sst:yt:client:buttons"). The equivalent loggers added inyt-binding-actions.tsin this same PR were renamed fromsst:*toapp:*per guideline. This introduces an inconsistent debug namespace within the same feature.As per coding guidelines, "Use
Debug('app:category:functionName')for debug logging... Prefix debug logs withapp:to distinguish application logs from third-party library logs."🧹 Proposed fix
-const debugClient = Debug("sst:yt:client") -const debugClientButtons = Debug("sst:yt:client:buttons") +const debugClient = Debug("app:yt:client") +const debugClientButtons = Debug("app:yt:client:buttons")🤖 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 `@apps/sst/src/routes/index.tsx` around lines 11 - 12, The new module-scope debug loggers in index.tsx use the old sst namespace, which is inconsistent with the app-prefixed logging convention used elsewhere in this feature. Update the Debug calls for debugClient and debugClientButtons to use the app: namespace pattern, matching the same naming style already applied in yt-binding-actions.ts and keeping the logging category consistent across the feature.Source: Coding guidelines
1220-1242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
not_foundfor mode toggles.field: "mode"only returnsupdatedornot_found;handleModeTogglecurrently drops thenot_foundcase, so a deleted tab leaves the stale UI in place and the toggle appears to do nothing. ReuseupdateFromWriteResult(result, "mode")or remove the tab onnot_found.🤖 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 `@apps/sst/src/routes/index.tsx` around lines 1220 - 1242, handleModeToggle currently ignores the not_found result from updateTabFieldFn when updating field "mode", which can leave a deleted tab displayed as if the toggle succeeded. Update the logic in handleModeToggle to reuse updateFromWriteResult(result, "mode") or explicitly handle result.status === "not_found" by removing the active tab / clearing stale UI state, while keeping the existing updated path and error handling intact.packages/yt-notes-scripts/src/expand_legacy_links.ts (1)
164-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--limit 0silently processes all candidates instead of none.
opts.limit ? ... : candidatestreats0as falsy, so--limit 0falls through to the fullcandidateslist rather than an empty slice — contradicting "Process only first N legacy links" and causing unintended stub writes.🐛 Proposed fix
- const trimmed = opts.limit ? candidates.slice(0, opts.limit) : candidates + const trimmed = + opts.limit !== undefined ? candidates.slice(0, opts.limit) : candidates🤖 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 `@packages/yt-notes-scripts/src/expand_legacy_links.ts` at line 164, The limit handling in expand_legacy_links.ts treats opts.limit as a truthy check, so --limit 0 incorrectly falls back to processing all candidates. Update the trimming logic around the candidates slice to explicitly check whether opts.limit is set (not just truthy) in the code path that builds trimmed, so a zero limit produces an empty list while positive limits still slice correctly.packages/yt-notes-scripts/README.md (1)
197-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale
--helpdump contradicts the fixed--templatedefault behavior.This copy-pasted help text still shows the old hardcoded Dropbox default for
--templateand omitsYT_TEMPLATE_PATHfrom the Environment list. The actualexpand_legacy_links.tsin this PR now sources the default fromprocess.env.YT_TEMPLATE_PATHand fails fast (exit 1) if neither is set — this doc block should be regenerated to match.📝 Proposed fix
- --template <path> Override template file path (default: - "/Users/hoschi/Library/CloudStorage/Dropbox/obsidian-test/test/yt-template.md") + --template <path> Template file path (default: $YT_TEMPLATE_PATH) -h, --help display help for command Environment: DATABASE_URL Postgres connection string YOUTUBE_API_KEY YouTube Data API v3 key (lazy fetch for missing details) + YT_TEMPLATE_PATH Path to the yt-template.md file (required if --template is omitted)🤖 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 `@packages/yt-notes-scripts/README.md` around lines 197 - 230, The `expand_legacy_links` help text in the README is stale and no longer matches the current CLI behavior. Regenerate the documented output for `expand_legacy_links.ts` so the `--template` option reflects the `YT_TEMPLATE_PATH`-backed default instead of the old hardcoded Dropbox path, and add `YT_TEMPLATE_PATH` to the Environment section alongside `DATABASE_URL` and `YOUTUBE_API_KEY`. Make sure the updated help block matches the current behavior of the `--template` handling and the fail-fast input error path when no template is configured.packages/yt-notes-scripts/src/import_youtube_history.ts (1)
45-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--limitto avoid silently importing everything.
parseIntreturnsNaNfor non-numeric input, and bothNaNand0are falsy — so at Line 85 (opts.limit ? entries.slice(...) : entries) a mistyped--limit abc(or--limit 0) silently falls through to processing all entries. Because the default path performs live DB writes, this turns an intended limited run into a full import.🛡️ Proposed fix
- .option("--limit <n>", "Process only first N entries", (v) => parseInt(v, 10)) + .option("--limit <n>", "Process only first N entries", (v) => { + const n = parseInt(v, 10) + if (!Number.isInteger(n) || n <= 0) { + console.error("Validation error: --limit must be a positive integer") + process.exit(3) + } + 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 `@packages/yt-notes-scripts/src/import_youtube_history.ts` at line 45, The --limit option in import_youtube_history.ts currently uses parseInt without validation, so invalid or zero values can fall through to the full-import path via opts.limit; update the option parsing and the subsequent limit handling around opts.limit to reject NaN/invalid input and treat 0 explicitly instead of relying on truthiness. Use the CLI option definition and the entries slicing logic as the main points to locate the fix.
🤖 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 `@packages/yt-notes-scripts/src/recompute-plain-from-srt.ts`:
- Around line 81-86: The `--limit` validation in `recomputePlainFromSrt` only
checks `Number.isFinite`, so negative values still pass and later break `take:
opts.limit`. Tighten the guard around `opts.limit` to enforce the documented
positive-integer contract (for example by rejecting non-integers and values less
than or equal to zero) before continuing, and keep the existing exit-1 behavior
with the same `console.error` path.
In `@packages/yt-notes-scripts/src/stub-creation.test.ts`:
- Around line 34-59: The Prisma test mock in stub-creation.test.ts is using an
unclear type-safety bypass with fakePrisma cast as never; update the mock typing
so it explicitly reads as a partial mock of PrismaClient instead. Change the
cast on fakePrisma to the more self-documenting double cast pattern used for
test doubles (as unknown as PrismaClient), keeping the existing
noteLink.findFirst and noteLink.upsert mock implementation intact.
---
Outside diff comments:
In `@apps/sst/src/routes/index.tsx`:
- Around line 11-12: The new module-scope debug loggers in index.tsx use the old
sst namespace, which is inconsistent with the app-prefixed logging convention
used elsewhere in this feature. Update the Debug calls for debugClient and
debugClientButtons to use the app: namespace pattern, matching the same naming
style already applied in yt-binding-actions.ts and keeping the logging category
consistent across the feature.
- Around line 1220-1242: handleModeToggle currently ignores the not_found result
from updateTabFieldFn when updating field "mode", which can leave a deleted tab
displayed as if the toggle succeeded. Update the logic in handleModeToggle to
reuse updateFromWriteResult(result, "mode") or explicitly handle result.status
=== "not_found" by removing the active tab / clearing stale UI state, while
keeping the existing updated path and error handling intact.
In `@packages/yt-notes-scripts/README.md`:
- Around line 197-230: The `expand_legacy_links` help text in the README is
stale and no longer matches the current CLI behavior. Regenerate the documented
output for `expand_legacy_links.ts` so the `--template` option reflects the
`YT_TEMPLATE_PATH`-backed default instead of the old hardcoded Dropbox path, and
add `YT_TEMPLATE_PATH` to the Environment section alongside `DATABASE_URL` and
`YOUTUBE_API_KEY`. Make sure the updated help block matches the current behavior
of the `--template` handling and the fail-fast input error path when no template
is configured.
In `@packages/yt-notes-scripts/src/expand_legacy_links.ts`:
- Line 164: The limit handling in expand_legacy_links.ts treats opts.limit as a
truthy check, so --limit 0 incorrectly falls back to processing all candidates.
Update the trimming logic around the candidates slice to explicitly check
whether opts.limit is set (not just truthy) in the code path that builds
trimmed, so a zero limit produces an empty list while positive limits still
slice correctly.
In `@packages/yt-notes-scripts/src/import_youtube_history.ts`:
- Line 45: The --limit option in import_youtube_history.ts currently uses
parseInt without validation, so invalid or zero values can fall through to the
full-import path via opts.limit; update the option parsing and the subsequent
limit handling around opts.limit to reject NaN/invalid input and treat 0
explicitly instead of relying on truthiness. Use the CLI option definition and
the entries slicing logic as the main points to locate the fix.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bfa7533-2bd7-4057-8f53-5201381c2869
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
apps/financy-forecast/package.jsonapps/financy-forecast/src/domain/snapshots.test.tsapps/sst/.env.baseapps/sst/.env.exampleapps/sst/package.jsonapps/sst/src/components/youtube-modal.tsxapps/sst/src/data/correct-with-claude.test.tsapps/sst/src/data/correct-with-claude.tsapps/sst/src/data/tab-sync-actions.tsapps/sst/src/data/text-improvement-actions.tsapps/sst/src/data/yt-binding-actions.tsapps/sst/src/data/yt-context.tsapps/sst/src/lib/title-truncation.test.tsapps/sst/src/lib/title-truncation.tsapps/sst/src/routes/index.tsxpackages/yt-notes-scripts/README.mdpackages/yt-notes-scripts/package.jsonpackages/yt-notes-scripts/prisma.config.tspackages/yt-notes-scripts/src/check-nightly-health.test.tspackages/yt-notes-scripts/src/check-nightly-health.tspackages/yt-notes-scripts/src/check-plugin-drift.tspackages/yt-notes-scripts/src/classify_channels.tspackages/yt-notes-scripts/src/cross-vault-link-validator.tspackages/yt-notes-scripts/src/db.tspackages/yt-notes-scripts/src/enrich-passes/pass1-audit.tspackages/yt-notes-scripts/src/enrich-passes/pass3-display-title.tspackages/yt-notes-scripts/src/enrich-passes/pass5-summary-long.tspackages/yt-notes-scripts/src/enrich-pipeline.tspackages/yt-notes-scripts/src/enrich-status.tspackages/yt-notes-scripts/src/enrich-video.tspackages/yt-notes-scripts/src/expand_legacy_links.tspackages/yt-notes-scripts/src/git-commit-helper.test.tspackages/yt-notes-scripts/src/git-commit-helper.tspackages/yt-notes-scripts/src/import_youtube_history.tspackages/yt-notes-scripts/src/import_youtube_transcript.tspackages/yt-notes-scripts/src/llm-caller.tspackages/yt-notes-scripts/src/markdown-parser.test.tspackages/yt-notes-scripts/src/markdown-parser.tspackages/yt-notes-scripts/src/recompute-plain-from-srt.e2e.tspackages/yt-notes-scripts/src/recompute-plain-from-srt.tspackages/yt-notes-scripts/src/relink-timestamps.tspackages/yt-notes-scripts/src/relocate_plugin_outputs.tspackages/yt-notes-scripts/src/stub-creation.test.tspackages/yt-notes-scripts/src/stub-creation.tspackages/yt-notes-scripts/src/stub-path-finder.tspackages/yt-notes-scripts/src/utils/file.test.tspackages/yt-notes-scripts/src/utils/file.tspackages/yt-notes-scripts/src/utils/obsidian-url-patcher.test.tspackages/yt-notes-scripts/src/utils/obsidian-url-patcher.tspackages/yt-notes-scripts/src/utils/yt-marker.tspackages/yt-notes-scripts/src/yt-lookup.tsturbo.json
- recompute-plain-from-srt.ts: reject negative and non-integer --limit values (Number.isFinite let -5 through, crashing Prisma take: -5); 0 stays valid - stub-creation.test.ts: cast the fakePrisma mock as unknown as PrismaClient instead of the opaque as never
buildPrivatePrompt and buildWorkPrompt duplicate most of their boilerplate and had drifted. Rather than extract shared constants (the prompts are kept self-contained for readability), the accidental drift is realigned against the canonical (more complete) private wording: rule 1 punctuation legend, rule 2 "mit ki" example + /URLs exclusion, rule 4 arrow-format noun list, rule 5 sentence-start example, rule 6 phonetic examples. The genuinely work-specific parts (YT-context priority rule + numbering shift, YT-context phonetic reference, handoff example) are unchanged. Sync-comments added to both builders pointing at each other so future edits update both.
Binding a YouTube URL runs a ~1-2 min critical enrich before finalizing; the
in-progress spinner lived only in client memory, so a reload during that window
lost it while the server kept working. bindYoutubeToTabFn now claims the tab
atomically before the enrich: updateMany where { id, youtubeId: null, mode:
"work" } sets youtube_id plus a new nullable bindingStartedAt marker; count 0
means the tab is already bound or being bound and returns a structured error.
Every post-claim failure path resets the claim (youtube_id + marker back to
null) so the tab stays re-bindable; both success paths clear the marker.
The tab snapshot now carries bindingInProgress, derived from bindingStartedAt
with a 10-minute stale cutoff for crash safety (in a prisma-free
binding-progress module with a unit test), and index.tsx re-derives the spinner
from it, so a reloaded client shows progress again.
The same atomic claim closes the TOCTOU race between the youtube_id existence
check and the write: two concurrent binds resolve to exactly one winner.
Adds migration add_tab_binding_started_at (nullable column, no backfill).
…etry hints formatRetryHint had a hardcoded "test" default and its only caller (enrich-pipeline.ts) never passed a name, so the retry hint always assumed the vault was named "test". The sharedVaultName parameter is now required; the caller sources the real name from yt.vault.name via the stub's StubLocation (findStubPath now returns vaultRef.name, already loaded via include). No new query, no env, no hardcoded vault name — the retry hint names the actual shared vault, so the tool works for any vault, not only one named "test".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/sst/src/data/yt-binding-actions.ts`:
- Around line 306-342: Add a stale-claim recovery path in the claim flow around
the atomic `sstPrisma.tab.updateMany` and `resetClaim` logic. Right now
`youtubeId` is only cleared on explicit error handling, so if a process dies
after setting `bindingStartedAt` the tab can stay stuck forever. Before
attempting a new claim, allow reclaiming rows whose `bindingStartedAt` is older
than `BINDING_STALE_CUTOFF_MS`, or add a cleanup step that clears `youtubeId`
and `bindingStartedAt` for stale bindings, while preserving the existing durable
claim/reset behavior.
In `@packages/yt-notes-scripts/src/cross-vault-link-validator.test.ts`:
- Around line 289-302: The `formatRetryHint` test block is wrapped in an
unnecessary `describe()` even though there is no shared setup or mock state.
Move the two independent `test()` cases out to top-level tests, removing the
`describe("formatRetryHint", ...)` wrapper and keeping the existing assertions
unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bd8de1f-bfb7-4ffc-934e-8a4b15ab3036
⛔ Files ignored due to path filters (6)
apps/sst/src/generated/prisma/commonInputTypes.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/class.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/prismaNamespace.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/internal/prismaNamespaceBrowser.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/models/Tab.tsis excluded by!**/generated/**apps/sst/src/generated/prisma/models/TabSyncState.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
apps/sst/prisma/migrations/20260705140424_add_tab_binding_started_at/migration.sqlapps/sst/prisma/schema.prismaapps/sst/src/contracts/tab-sync.tsapps/sst/src/data/tab-sync-actions.tsapps/sst/src/data/yt-binding-actions.tsapps/sst/src/lib/binding-progress.test.tsapps/sst/src/lib/binding-progress.tsapps/sst/src/routes/index.tsxpackages/yt-notes-scripts/src/cross-vault-link-validator.test.tspackages/yt-notes-scripts/src/cross-vault-link-validator.tspackages/yt-notes-scripts/src/enrich-pipeline.test.tspackages/yt-notes-scripts/src/enrich-pipeline.tspackages/yt-notes-scripts/src/stub-path-finder.ts
The atomic bind claim required youtube_id to be null, so a process that died after the claim (youtube_id + bindingStartedAt set) but before finalize/ resetClaim left the tab permanently un-bindable. The claim where-clause now also matches a work-mode tab whose bindingStartedAt is older than BINDING_STALE_CUTOFF_MS, so a fresh bind reclaims a stale one. A finalized tab (bindingStartedAt null) is never matched (lt on NULL is never true in Postgres), and a live in-progress bind (recent bindingStartedAt) is not old enough — so neither is ever stolen. Extracted buildStaleClaimWhere into the prisma-free binding-progress module with unit tests.
The describe block had no shared setup; its two cases are now flat top-level test() calls per the repo convention.
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
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.
YouTube-Notizen-Pipeline mit LLM-Enrichment und Speech-to-Text-Arbeitsmodus mit YouTube-Kontext-Anreicherung.
Import: Google-Takeout-Watch-History, cookieless Transcript-Fetch, Note-Links aus dem Vault, Channel-Klassifikation (arbeit/privat)
Enrichment-Pipeline: mehrstufige LLM-Passes (Titel, Beschreibung, Zusammenfassung, Verwandt-Sektion), Timestamp-Linkify
Pass 5 ruft einen externen Such-Wrapper (OHS_WRAPPER_PATH) zur Cross-Vault-Wikilink-Disambiguierung auf (der Suchindex selbst liegt außerhalb dieses Repos)
Vault-Stub-Generierung mit Frontmatter (youtube_id) + Cross-Vault-Wikilinks
Health-/Morgen-Check (check-nightly-health) mit Failure-Tracking
youtube-context-lookup Skill + yt-lookup CLI
Speech-to-Text-Arbeitsmodus mit YouTube-Kontext (Eigennamen-Korrektur)
Kritische-Fehler- und Background-Status-Anzeige
yt-Schema + konsolidierte Init-Migration
Summary by CodeRabbit