refactor: extract ArtifactGenerationService to drain _artifacts.py below budget#1565
Conversation
|
@claude review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR extracts artifact-generation kickoff logic from the ChangesArtifact Generation Service Extraction
Sequence DiagramssequenceDiagram
participant Service as ArtifactGenerationService
participant RPC as RpcCaller
participant Parser as _parse_generation_result
Service->>Service: _call_generate()
Service->>RPC: CREATE_ARTIFACT RPC
RPC-->>Service: response or null
alt null response with feature gate
Service->>Service: raise ArtifactFeatureUnavailableError
else success response
Service->>Parser: _parse_generation_result()
Parser-->>Service: GenerationStatus
end
sequenceDiagram
participant Client as Client/User
participant Facade as ArtifactsAPI
participant Service as ArtifactGenerationService
Client->>Facade: generate_audio/video/report/etc
Facade->>Service: _generation.generate_audio/video/report/etc
Service->>Service: resolve source_ids, build params
Service->>Service: _call_generate
Service-->>Facade: GenerationStatus
Facade-->>Client: GenerationStatus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
Claude finished @teng-lin's task in 3m 37s —— View job Code ReviewTasks:
SummaryThis is a clean, well-executed extraction. The Issues1.
|
teng-lin
force-pushed
the
refactor/extract-artifact-gen
branch
from
3825ae7 to
c6a17ad
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to extract artifact generation logic into a new ArtifactGenerationService and source upload decode/validation helpers into _source/_upload_decode.py. This modularization successfully reduces the size of several oversized modules below the 1000-line budget, allowing their removal from the module size ratchet allowlist. Feedback on the changes suggests replacing instances of next(iter(sequence)) with explicit sequence unpacking in _upload_decode.py, downloads.py, and generation.py to make single-element invariants explicit and avoid potential StopIteration issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
src/notebooklm/_source/_upload_decode.py (108-109)
Following the project's general rules, prefer sequence unpacking over next(iter(sequence)) when retrieving the first or only element from a sequence. Since upload_ids is guaranteed to have exactly one element after the length check, we can explicitly unpack it to make the single-element invariant explicit and avoid potential StopIteration issues.
if len(upload_ids) != 1:
raise ValidationError("Upload URL must include exactly one non-empty upload_id")
(upload_id,) = upload_ids
if not upload_id:
raise ValidationError("Upload URL must include exactly one non-empty upload_id")
References
- In Python, when retrieving the first or only element from a sequence, prefer sequence unpacking over next(iter(sequence)) to make the single-element invariant explicit and avoid StopIteration issues.
src/notebooklm/_artifact/downloads.py (901-902)
Following the project's general rules, prefer sequence unpacking over next(iter(sequence)) when retrieving the first or only element from a sequence. Since writer_error is a one-slot exception box, we can explicitly unpack it to make the single-element invariant explicit.
if writer_error:
(err,) = writer_error
raise err
References
- In Python, when retrieving the first or only element from a sequence, prefer sequence unpacking over next(iter(sequence)) to make the single-element invariant explicit and avoid StopIteration issues.
src/notebooklm/_artifact/generation.py (535-540)
To adhere to the project's guidelines of avoiding next(iter(sequence)) for retrieving the first element of a sequence (especially to prevent StopIteration issues), we can use sequence unpacking when the slice is non-empty. This also respects the custom linter constraint against direct integer indexing.
# Best-effort debug label over the OUTGOING request body; the ``[2:3]``
# slice-pick keeps it off ``name[int]`` (== old guarded ``params[2]``).
descriptor = None
if params[2:3]:
(descriptor,) = params[2:3]
artifact_type = "unknown"
if isinstance(descriptor, list) and descriptor[2:3]:
(artifact_type,) = descriptor[2:3]
References
- In Python, when retrieving the first or only element from a sequence, prefer sequence unpacking over next(iter(sequence)) to make the single-element invariant explicit and avoid StopIteration issues.
|
Addressed in 4f7f936: @claude (reach-in guardrail gap):
@gemini-code-assist (
|
|
Thanks for the update, @teng-lin. The guardrail registrations and the docstring refresh look correct and complete. Since you've verified that |
…low budget Extract the generation kickoff paths (generate_*, revise_slide, retry_failed, plus _call_generate / _parse_generation_result) from the ArtifactsAPI facade into a new ArtifactGenerationService (_artifact/generation.py), a peer to ArtifactDownloadService and ArtifactPollingService. The facade keeps thin, signature-preserving delegators (including _call_generate / _parse_generation_result, which tests call directly) so the public API and patch seams are unchanged. Combined with aggressive docstring/comment trimming on the facade, this drains _artifacts.py from 1447 to 990 lines, below the 1000-line module-size budget, and removes its ALLOWLISTED_CEILINGS entry (one-way ratchet) — leaving exceptions.py as the sole allowlisted module. Issue/ADR refs and positional RPC-shape rationale were preserved. docs/architecture.md updated (class index, file index, repo tree, and the artifact-generation call-flow row). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ards - Register _artifact/generation.py in _ARTIFACT_SERVICE_MODULES and _REACH_IN_MIGRATED_MODULES (test_no_facade_reach_in.py) so the facade-reach-in / facade-import drift guards actively cover the new service, and refresh the stale _ApiReachInVisitor docstring that still claimed the old _artifact_generation.py was folded into the facade (#1205). - In ArtifactGenerationService._call_generate, unpack the at-most-one-element param slices instead of next(iter(...)) so the single-element invariant is explicit (per gemini-code-assist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
teng-lin
force-pushed
the
refactor/extract-artifact-gen
branch
from
4f7f936 to
1dbbee6
Compare
teng-lin
changed the base branch from
refactor/drain-module-size-ceilings
to
main
Empty commit to fire the synchronize event so test.yml runs natively on the PR now that its base is main (retargeting via API did not re-fire CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Extracts the artifact generation kickoff paths from the 1447-line
ArtifactsAPIfacade into a newArtifactGenerationService(src/notebooklm/_artifact/generation.py) — a peer to the existingArtifactDownloadServiceandArtifactPollingService. Combined with aggressive docstring/comment trimming on the facade, this drains_artifacts.pyto 990 lines, below the 1000-line module-size budget, and removes itsALLOWLISTED_CEILINGSentry (one-way ratchet) — leavingexceptions.pyas the sole allowlisted module.What moved
Into
ArtifactGenerationService(rpc=…, notebooks=…, note_service=…):generate_audio/video/cinematic_video/report/study_guide/quiz/flashcards/infographic/slide_deck/data_table/mind_maprevise_slide,retry_failed_call_generate,_parse_generation_resultThe facade keeps thin, signature-preserving delegators for every public method.
_call_generateand_parse_generation_resultalso keep facade hops because tests call them directly on the API instance.Why it's seam-safe
setattr/patch.objecton_call_generate/_parse_generation_result; they're called directly (preserved by the delegators) or exercised via a mocked RPC.mock_core.rpc_executor.rpc_call) via.side_effect/.return_value, so the extracted service sees it.generate_*patch-seam tests pass unchanged — no seam repoints needed.What did NOT move
_is_media_ready,_extract_artifact_error,_get_artifact_type_namestay on the facade (status/polling concerns delegating to_artifact_polling). Their verbose docstrings were trimmed but the positional RPC-shape rationale (art[3]/art[5]/art[6][5]/…) was kept. Every issue/ADR ref (#1342, #1270, #1491, #1247, #1362, ADR-0019, …) was preserved.Verification
pytest tests/unit tests/integration -k "artifact or generation or mind_map or generate"→ 1543 passedpytest tests/_guardrails/test_module_size_ratchet.py test_public_surface_manifest.py tests/unit/test_claude_md_freshness.py→ 309 passedbrowserextra in the sandbox, unrelated; green once installed)mypy src/notebooklm --ignore-missing-imports→ cleanpre-commit run --all-files→ passscripts/audit_public_api_compat.py→ exit 0 (non-breaking; no allowlist entries added)Docs
docs/architecture.mdupdated: class-index table, file-index table, repo tree, and the artifact-generation call-flow row (which previously said the service "was folded into the facade").🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Chores