Conversation
Extension, body_format, and frontmatter for each slide framework were scattered across cli.rb and markdown_renderer.rb. Move them into a single Slidict::Output::Format registry so adding a new framework only requires one new entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Slidict::CLI, SlidesCommand, and Server were flat siblings of domain classes (Deck, Config, LLMClient, etc.), making it unclear which classes are CLI entry points versus reusable domain logic. Move them into lib/slidict/cli/ as Slidict::Cli::App, Slidict::Cli::Slides, and Slidict::Cli::Serve. Also drop Server's unused class-level .run, since the instance method was already the single call path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AuthClient and SlidesClient both talk directly to the slidict.io HTTP API, but lived as flat siblings of unrelated domain classes. Move them into Slidict::External (auth_client.rb, slides_client.rb) so the boundary between "talks to slidict.io" and local-only logic (Credentials, Cli::*) is explicit in the namespace. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MarkdownRenderer turns a Deck into the rendered output, so it belongs next to Output::Format rather than as a flat sibling of CLI/auth code. Renamed to Slidict::Output::Renderer to match the namespace. Deck and Slide stay at the Slidict top level since they're the domain model fed into both the LLM client and the renderer, not output themselves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
build_app referenced Sinatra::Base directly, relying on run to have required "sinatra/base" first. Calling build_app on its own (as serve_spec.rb does to test the Rack app without starting a server) raised NameError. Move the require into build_app so it works either way.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 Walkthroughウォークスルー旧来の単一ファイル実装（ 変更点CLI・外部クライアント・出力レンダラのモジュール再編と serve サブコマンド追加
シーケンス図sequenceDiagram
participant User as ユーザー
participant App as Cli::App
participant Auth as External::SlidictIo::Auth
participant Creds as External::SlidictIo::Credentials
participant LLM as Llm::Client
participant Renderer as Output::Renderer
participant Slides as Cli::Slides
participant Serve as Cli::Serve
User->>App: slidict auth
App->>Auth: request_device_code
Auth-->>App: device_code, user_code
App->>Auth: poll_token(device_code:)
Auth-->>App: access_token
App->>Creds: write_cli_token!
User->>App: slidict --topic "..."
App->>LLM: verify_connection!
LLM-->>App: ok
App->>LLM: generate_slides(deck)
LLM-->>App: Slide[]
App->>Renderer: render(deck)
Renderer-->>App: content
App-->>User: Created public/001.md
User->>App: slidict serve
App->>Serve: run(args)
Serve-->>User: Sinatra GET /
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~60 minutes 関連する可能性のある PR
提案ラベル
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Matches the Cli/External/Output namespace pattern already used elsewhere. Updated the .rubocop.yml ClassLength exclude path to follow the file move. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@lib/slidict/cli/app.rb`:
- Around line 282-295: `output_path_for` currently always prefixes `public/`, so
passing `--filename public/...` in `normalize_filename` produces a duplicated
`public/public/...` path. Update the path handling in `output_path_for` and/or
`normalize_filename` to either strip an initial `public/` segment before joining
or reject such input explicitly, and keep the validation consistent with the
existing `ArgumentError` checks for relative paths and `..`.
In `@lib/slidict/cli/serve.rb`:
- Around line 69-70: The escape_path method in serve.rb is using CGI.escape on
each path segment, which turns spaces into plus signs and breaks static file
href matching. Update escape_path to use a path-segment-safe encoder that
preserves spaces as percent-encoding, and ensure the Serve CLI path handling
still joins segments correctly. Add a regression spec around the escape_path
behavior or the serving path for a filename like my slide.md to verify the href
becomes the expected encoded path.
In `@lib/slidict/cli/slides.rb`:
- Around line 100-103: `fetch_value!` is rejecting any argument that starts with
`-`, which breaks valid CLI values like `--title "-draft"` or `--body "---..."`.
Update the option parsing in `Slides::CLI#fetch_value!` so it only treats a
leading dash as missing when the token is actually another known option or a
true absent value, and allow legitimate string values that begin with `-` to
pass through for `slidict slides create/edit` as well as `publish`.
- Around line 169-170: `read_body` で `options[:file]` を `File.read`
している箇所は、存在しないパスや権限不足の `Errno::*` がそのまま未処理で落ちるため、`Slides::CLI#read_body`
内で読み込み失敗を捕捉して `ArgumentError` に変換し、`run` 側の既存エラーハンドリングに流すようにしてください。`read_body` と
`run` の責務を崩さず、`--file` 指定時の失敗が CLI 全体の例外にならないようにしてください。
- Around line 107-122: The `list` and `show` methods in `SlidesCLI` currently
skip the 401 re-auth retry path that `submit` uses, so expired tokens fall
straight into `print_client_error`. Update these command handlers to catch
`External::SlidesClient::Unauthorized`, trigger the same
re-authentication-and-retry flow used by the create/edit path, and only fall
back to `External::SlidesClient::Error` handling after retry fails. Keep the
retry logic aligned with the existing client calls in `list`, `show`, and the
submit flow so behavior is consistent across subcommands.
In `@lib/slidict/external/auth_client.rb`:
- Around line 57-59: The Net::HTTP call in auth_client’s request path has no
timeout settings, so the CLI can hang during authentication. Update the
Net::HTTP.start usage in the auth client flow (including the methods that drive
request_device_code and poll_token) to set explicit open_timeout and
read_timeout on the HTTP object before making the request. Keep the change
localized to the HTTP request helper so all auth API calls inherit the same
timeout behavior.
In `@lib/slidict/external/slides_client.rb`:
- Around line 90-92: The Net::HTTP request in SlidesClient currently has no
timeout, so a stalled API can hang the CLI indefinitely. Update the SlidesClient
request path that uses Net::HTTP.start and http.request to set both open_timeout
and read_timeout on the HTTP object before making the call. Keep the change
localized to the slide API client so the failure mode for slidict slides stays
bounded.
🪄 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: CHILL
Plan: Pro
Run ID: 55af38b0-144b-421f-be15-0742f6a4acf6
📒 Files selected for processing (23)
.gitignoreREADME.mdbin/slidictlib/slidict.rblib/slidict/auth_client.rblib/slidict/cli.rblib/slidict/cli/app.rblib/slidict/cli/serve.rblib/slidict/cli/slides.rblib/slidict/external/auth_client.rblib/slidict/external/slides_client.rblib/slidict/markdown_renderer.rblib/slidict/output/format.rblib/slidict/output/renderer.rblib/slidict/slides_client.rblib/slidict/slides_command.rblib/slidict/version.rbslidict.gemspecspec/slidict/cli/app_spec.rbspec/slidict/cli/serve_spec.rbspec/slidict/cli/slides_spec.rbspec/slidict/external/slides_client_spec.rbspec/slidict/output/renderer_spec.rb
💤 Files with no reviewable changes (5)
- lib/slidict/cli.rb
- lib/slidict/auth_client.rb
- lib/slidict/slides_command.rb
- lib/slidict/markdown_renderer.rb
- lib/slidict/slides_client.rb
Comment on lines
+57
to
+59
| response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http| | ||
| http.request(request) | ||
| end |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
認証 API 呼び出しにタイムアウトがなく、CLI が張り付く可能性があります。
接続先のハングやネットワーク断で request_device_code / poll_token が返らなくなります。認証フローは対話コマンドなので、open_timeout / read_timeout はここで明示した方が安全です。
修正案
- response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http|
+ response = Net::HTTP.start(
+ uri.hostname,
+ uri.port,
+ use_ssl: uri.scheme == "https",
+ open_timeout: 5,
+ read_timeout: 30
+ ) do |http|
http.request(request)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http| | |
| http.request(request) | |
| end | |
| response = Net::HTTP.start( | |
| uri.hostname, | |
| uri.port, | |
| use_ssl: uri.scheme == "https", | |
| open_timeout: 5, | |
| read_timeout: 30 | |
| ) do |http| | |
| http.request(request) | |
| end |
🤖 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 `@lib/slidict/external/auth_client.rb` around lines 57 - 59, The Net::HTTP call
in auth_client’s request path has no timeout settings, so the CLI can hang
during authentication. Update the Net::HTTP.start usage in the auth client flow
(including the methods that drive request_device_code and poll_token) to set
explicit open_timeout and read_timeout on the HTTP object before making the
request. Keep the change localized to the HTTP request helper so all auth API
calls inherit the same timeout behavior.
Comment on lines
+90
to
+92
| response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http| | ||
| http.request(request) | ||
| end |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
slides API 呼び出しにもタイムアウトがありません。
このままだと API 側の停止や TCP ハングで slidict slides が戻らなくなります。少なくとも open_timeout / read_timeout を入れて、対話 CLI の失敗モードを有限にしてください。
修正案
- response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http|
+ response = Net::HTTP.start(
+ uri.hostname,
+ uri.port,
+ use_ssl: uri.scheme == "https",
+ open_timeout: 5,
+ read_timeout: 30
+ ) do |http|
http.request(request)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http| | |
| http.request(request) | |
| end | |
| response = Net::HTTP.start( | |
| uri.hostname, | |
| uri.port, | |
| use_ssl: uri.scheme == "https", | |
| open_timeout: 5, | |
| read_timeout: 30 | |
| ) do |http| | |
| http.request(request) | |
| end |
🤖 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 `@lib/slidict/external/slides_client.rb` around lines 90 - 92, The Net::HTTP
request in SlidesClient currently has no timeout, so a stalled API can hang the
CLI indefinitely. Update the SlidesClient request path that uses Net::HTTP.start
and http.request to set both open_timeout and read_timeout on the HTTP object
before making the call. Keep the change localized to the slide API client so the
failure mode for slidict slides stays bounded.
Credentials exists to support authenticating against the slidict.io API (it stores/reads the CLI access token used by AuthClient and SlidesClient), so it belongs alongside them in Slidict::External rather than as a flat sibling of CLI-only code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AuthClient, SlidesClient, and Credentials all exist specifically to talk to (or hold credentials for) the slidict.io API, so nest them under Slidict::External::SlidictIo as Auth, Client, and Credentials rather than flat siblings under External. Leaves External free for any other third-party integration added later. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- cli/app.rb: --filename public/x no longer duplicates into public/public/x.md - cli/serve.rb: percent-encode hrefs instead of CGI.escape, which turned spaces into "+" and broke static file lookups - cli/slides.rb: fetch_value! no longer rejects legitimate values that start with "-" (e.g. YAML frontmatter bodies) for create/edit - cli/slides.rb: list/show now share the same 401 reauthenticate-and- retry behavior as create/edit, instead of failing immediately - cli/slides.rb: read_body converts Errno::ENOENT/EACCES from a missing/unreadable --file into a clean ArgumentError instead of crashing the CLI - external/slidict_io/auth.rb, client.rb: set open_timeout/read_timeout on Net::HTTP.start so a hung slidict.io endpoint can't block the CLI forever Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
slidict serveで生成済みスライドをローカル配信できるようになりました。slidict slidesで一覧表示・詳細表示・作成・編集ができるようになりました。public/配下に変更され、フレームワーク別の出力形式も整理されました。Bug Fixes
Chores
0.3.1に更新しました。