Skip to content

feat(job): print dashboard preview link#596

Open
kannwism wants to merge 1 commit into
mainfrom
feat(job)/print-dashboard-preview-link
Open

feat(job): print dashboard preview link#596
kannwism wants to merge 1 commit into
mainfrom
feat(job)/print-dashboard-preview-link

Conversation

@kannwism
Copy link
Copy Markdown
Contributor

Summary

  • print a dashboard preview URL after creating a job definition
  • keep the existing phone QR code preview unchanged

Validation

  • git diff --check
  • PATH="/home/agent/.local/share/mise/installs/node/20.20.2/bin:$PATH" pyright src/rapidata/rapidata_client (fails: dependencies are not installed in this container, causing repo-wide missing-import errors for pydantic, pandas, opentelemetry, etc.)

🔗 Session: session-56ddff8d

Co-Authored-By: RapidPoseidon <poseidon@rapidata.ai>
@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Code Review

Overview

This PR adds a dashboard URL preview link printed to the terminal after creating a job definition. The implementation is clean and minimal - two small functions in _qr_preview.py and a single call site in _create_general_job_definition.


What works well

  • Respects silent_mode, consistent with all other print helpers in this module.
  • Uses managed_print and follows the same pattern as the existing campaign preview helpers.
  • Good separation between the URL builder (build_job_definition_preview_url) and the printing function - the builder is independently testable and reusable.
  • No unnecessary abstraction or over-engineering.

Issues / Suggestions

1. Module docstring is now stale

The module-level docstring at the top of _qr_preview.py still reads:

Helpers for printing a terminal-friendly QR code that links to the public campaign preview page.

Now that the module also handles plain URL link printing for job definitions, this should be updated to reflect the broader scope.


2. No error handling - inconsistent with the module's stated contract

The module docstring explicitly promises best-effort semantics:

failures are logged at debug level and never raised to the caller, so order / job execution is never affected

print_job_definition_preview_link is the only print helper in the file that does not wrap its body in a try/except. While managed_print on a formatted string is extremely unlikely to fail, it still breaks the contract stated for this module. For consistency, the body should be wrapped in a try/except with logger.debug on failure, matching the pattern used throughout the rest of the module.


3. Print message wording inconsistency

The fallback in print_campaign_preview_qr uses "Preview at: {url}", while the new function uses "Preview in dashboard: {url}". The difference may be intentional (different surfaces), but it is worth confirming this is a deliberate UX choice and not an oversight.


4. Ordering: QR code then link

In _create_general_job_definition, the campaign QR is printed before the dashboard link. The QR lookup involves retries via _resolve_campaign_id_from_pipeline, so there can be a noticeable delay before the dashboard link appears. Worth verifying the UX feels coherent in practice.


5. Pyright could not be validated

The PR notes pyright could not run due to missing deps in the container. The key type to verify is job_model.id - if RapidataJobDefinition.id is typed as str | None, passing it to build_job_definition_preview_url(job_definition_id: str) would be a type error. Worth confirming this locally before merging.


Summary

Small, clean addition. The main gap is the missing try/except wrapper (for consistency with the module's own best-effort contract) and the stale module docstring. Everything else looks good.

@kannwism kannwism requested a review from LinoGiger May 20, 2026 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant